atomsGaffer icon indicating copy to clipboard operation
atomsGaffer copied to clipboard

Minor performances fix and a small bug fix

Open fdagenais-cinesite opened this issue 3 years ago • 3 comments

First of all, this branch contains some minor performance fixes:

  1. For this one, the credits go to Alexander who pointed it out. In most cases, an agent id's will match its index in the agents list. When fetching an agent id from its index, we can save time by first looking if the index matches the id. This avoids having to loop over all entries in the vector, which does not scale well as the number of agents increases.
  2. Most of the time is generally spent computing the skin deformation. During that operation, roughly 5% of the time was spent on non required normalization of 4D vectors. These extra computations have been removed. In my test with ~750 agents, this was roughly ~2s, which is not much, but as the number of agents and/or their mesh complexity increases, this could save some more time.
  3. Loading the scene hierarchy of the AtomsVariationReader or AtomsCrowdGenerator nodes in Gaffer, it can take a few seconds to process. Most of the time is spent on loading the atoms meshes in AtomsVariationReader. To speed things up, all meshes from one agent are now loaded in parallel, which significantly improves responsiveness. Note that the gain is limited when processing more that the atoms node, since other nodes might be processed on other cores, e.g., during a render. I wasn't sure this was really needed, but it does improve things when looking directly at the Atoms nodes. I can remove Also note that this is a quick fix, but a better solution, which would take more time to implement, would be to change the way we load atoms data. Instead of loading everything at once, we could load only the required information to generate the child names, bbox, etc., and then load the meshes only when requested.

Also, this branch contains a small bug fix I found while looking at the skin deformation code in AtomsCrowdGenerator. Instead of using the normals matrix's inverse to transform the normals back to local space, the normals matrix (local to world) is used instead. I believe this has not been detected before because the agent's transformation matrices only contained translation information, with the inner 3x3 matrix being the identity.

fdagenais-cinesite avatar Feb 08 '21 17:02 fdagenais-cinesite

Reviewers comments have been handled in latest commit:

  • The std::cout call that was left over has been removed.
  • The engineDataPlug() cache policy has been changed to TaskCollaboration.
  • The atoms mesh loading tbb loop is now run in it's own isolated context group.

Note: I did not add a way to disable Normals transformation during skin deformation because I was not sure if there was a safe way to guarantee that it won't be used. If you want, I can still add an option that can be toggled by the user to enable/disable normals transformation.

fdagenais-cinesite avatar Feb 23 '21 15:02 fdagenais-cinesite

Sorry, to clarify about my skipping N comment, I mistakenly thought we had an opt-out plug on AtomsVariationReader which would skip loading them here, but checking now I see we only have that for Pref & Nref... If we had an additional toggle for N then the AtomsCrowdGenerator wouldn't need any changes because it already returns early if they are missing.

In any case, its not related to this PR, just a thought for the future.

andrewkaufman avatar Feb 23 '21 16:02 andrewkaufman

Latest changes look good to me.

Another thing you might want to consider for the future is adding support for cancellation. This is the mechanism Gaffer uses to abort long running operations and make the UI responsive again. You do that by calling IECore::Canceller::check( context->canceller() ) periodically in your long compute. If cancellation has been requested, that'll throw an IECore::Cancelled exception which will abort the computation all the way back to the caller. Your only other responsibility is to make sure your code is exception-safe. Here's a super simpler example :

https://github.com/GafferHQ/gaffer/blob/c908425b6d2e82105cd92a1398fae8eeb0ca9600/src/GafferImage/Resample.cpp#L552

johnhaddon avatar Feb 23 '21 16:02 johnhaddon