Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary loop sending octree packets to a known node. #1353

Merged
merged 2 commits into from Feb 10, 2022

Conversation

odysseus654
Copy link
Contributor

@odysseus654 odysseus654 commented Sep 19, 2021

OctreeEditPacketSender can either send a packet (list) to a specific node or all nodes. In all cases it is currently looping through all known nodes and checking to see if they match the filter criteria before processing the request.

This patch splits the search into a for loop if doing all nodes and a direct-retrieve if the node is known.

This should be a very small optimization (saving for loop and functor call over every known node) but it might be significant if this function gets called a lot (once for every entity change? It's heavily optimized down the line already) and with a larger number of active users.

I have not done any testing outside of an interface smoketest (which doesn't even touch this line of code).

Testing focus: This patch changes the code responsible for changing entities in the domain, both storing messages before a user is fully connected and sending packets once they are. The actions affected by this change are:

  • PacketType::EntityAdd - an entity with the specified properties is added to the domain
  • PacketType::EntityEdit - an entity has had one or more properties changed (e.g. edited or changed by script)
  • PacketType::EntityPhysics - an entity has had one or more properties changed by the physics engine
  • PacketType::EntityErase - an entity has been removed from the domain
  • PacketType::EntityClone - an entity has been cloned from an existing entity in the domain

@odysseus654 odysseus654 changed the title [networking][performance] remove unnecessary loop sending octree packets to a known domain [networking][performance] remove unnecessary loop sending octree packets to a known node Sep 19, 2021
@digisomni digisomni added domain-server Issues related to the domain server in general enhancement New feature or request labels Sep 19, 2021
@digisomni digisomni added this to In progress in 2022.1.0 Selene Release via automation Sep 19, 2021
@digisomni digisomni added this to the 2021.2.0 Selene Release milestone Sep 19, 2021
2022.1.0 Selene Release automation moved this from In progress to Reviewer approved Sep 19, 2021
@digisomni digisomni added CR Approved At least one code reviewer has approved the PR. needs testing (QA) The PR is ready for testing and removed needs CR (code review) labels Sep 19, 2021
@digisomni digisomni changed the title [networking][performance] remove unnecessary loop sending octree packets to a known node [networking][performance] Remove unnecessary loop sending octree packets to a known node. Sep 23, 2021
@digisomni
Copy link
Member

Testing: Please mess with entities on a local domain to see if anything broke, if everything works as expected then the PR should be fine. :)

@odysseus654
Copy link
Contributor Author

Probably even better if two people are on the domain, one person messes with the entities and the other person should see the same messing.

@digisomni digisomni changed the title [networking][performance] Remove unnecessary loop sending octree packets to a known node. Remove unnecessary loop sending octree packets to a known node. Sep 30, 2021
@digisomni digisomni added the housekeeping Code or documentation cleanup label Oct 1, 2021
@daleglass daleglass added the deploy to test server A bot will see this tag, build and start a test server with it label Oct 21, 2021
@digisomni digisomni removed this from Reviewer approved in 2022.1.0 Selene Release Oct 30, 2021
@digisomni digisomni added this to In progress in 2022.1.1 Selene Release via automation Oct 30, 2021
@digisomni digisomni added rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Dec 4, 2021
@digisomni digisomni added allow-build-upload Allows the upload of a build for non white-listed users rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Jan 27, 2022
@digisomni
Copy link
Member

image

I think she works!

@digisomni digisomni added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Feb 7, 2022
@daleglass daleglass merged commit fcd6166 into vircadia:master Feb 10, 2022
2022.1.1 Selene Release automation moved this from In progress to Done Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allow-build-upload Allows the upload of a build for non white-listed users CR Approved At least one code reviewer has approved the PR. deploy to test server A bot will see this tag, build and start a test server with it domain-server Issues related to the domain server in general enhancement New feature or request housekeeping Code or documentation cleanup QA Approved The PR has been tested successfully. rebuild rebuild through the GithubActions
Development

Successfully merging this pull request may close these issues.

None yet

5 participants