gz-sim icon indicating copy to clipboard operation
gz-sim copied to clipboard

Published 20 hours ago • verified publisher icon gazebosim

Optimize performance by skipping scene update when possible

Open darksylinc opened this issue 2 years ago • 5 comments

Summary

IMPORTANT:

This PR depends on ignitionrobotics/ign-rendering#415 and should not be merged until that PR is.

Ray queries (performed every time the mouse moves) had a negative performance impact caused by https://github.com/ignitionrobotics/ign-rendering/pull/415

This commit prevents repetead calls to IgnRenderer::ScreenToScene to perform unnecessary performance degradation since calling ClosestPoint(true) the first time during the frame is enough

Code will not compile until PR ignitionrobotics/ign-rendering#415 is merged

Signed-off-by: Matias N. Goldberg [email protected]

Summary

Checklist

  • [x] Signed all commits for DCO
  • [ ] Added tests
  • [ ] Updated documentation (as needed)
  • [ ] Updated migration guide (as needed)
  • [x] codecheck passed (See contributing)
  • [ ] All tests passed (See test coverage)
  • [ ] While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

darksylinc avatar Sep 19 '21 00:09 darksylinc

Note: As I'm studying a few changes I noticed we may be able to optimize it back to pre ignitionrobotics/ign-rendering#415 levels by calling this->HandleMouseEvent(); either before this->dataPtr->renderUtil.Update(); or after this->dataPtr->camera->Update(); and thus always calling rayQuery->ClosestPoint(false)

This would cause mouse events to lag behind by 1 frame; but no update of scene would be required (thus no extra CPU needed). Without ignitionrobotics/ign-rendering#415; gazebo's GUI is currently operating on data that is behind by 1 frame anyway.

darksylinc avatar Sep 19 '21 14:09 darksylinc

Note that from Fortress, GzScene3D is being deprecated in favor of MinimalScene (still WIP), so we'll need to port this fix there). CC @ahcorde

chapulina avatar Sep 20 '21 16:09 chapulina

@darksylinc , are you still working on this?

chapulina avatar Apr 11 '22 17:04 chapulina

I forgot about this but because I thought it was already merged.

If I understand correctly this code should be ported to MinimalScene now?

darksylinc avatar Apr 12 '22 14:04 darksylinc

If I understand correctly this code should be ported to MinimalScene now?

Yes, please 🙇🏽‍♀️ You're welcome to leave the code in GzScene3D too because Fortress users may still be relying on it.

chapulina avatar Apr 12 '22 18:04 chapulina

Scene3D has been removed from the gz-sim7 branch. So this needs to target MinimalScene on gz-gui. Closing.

chapulina avatar Sep 01 '22 02:09 chapulina