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

Optimize performance by skipping scene update when possible #1037

Closed
wants to merge 1 commit into from

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Sep 19, 2021

Summary

IMPORTANT:

This PR depends on gazebosim/gz-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
gazebosim/gz-rendering#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 gazebosim/gz-rendering#415 is
merged

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

Summary

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

Ray queries (performed every time the mouse moves) had a negative
performance impact caused by
gazebosim/gz-rendering#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 gazebosim/gz-rendering#415 is
merged

Signed-off-by: Matias N. Goldberg <[email protected]>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 19, 2021
@darksylinc
Copy link
Contributor Author

darksylinc commented Sep 19, 2021

Note: As I'm studying a few changes I noticed we may be able to optimize it back to pre gazebosim/gz-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 gazebosim/gz-rendering#415; gazebo's GUI is currently operating on data that is behind by 1 frame anyway.

@chapulina chapulina added beta Targeting beta release of upcoming collection bug Something isn't working labels Sep 20, 2021
@chapulina chapulina added the rendering Involves Ignition Rendering label Sep 20, 2021
@chapulina
Copy link
Contributor

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

@ahcorde ahcorde added the needs upstream release Blocked by a release of an upstream library label Sep 20, 2021
@chapulina chapulina requested a review from iche033 September 20, 2021 18:57
@chapulina chapulina removed the beta Targeting beta release of upcoming collection label Sep 20, 2021
@chapulina chapulina added 🌱 garden Ignition Garden and removed 🏯 fortress Ignition Fortress labels Nov 16, 2021
@chapulina
Copy link
Contributor

@darksylinc , are you still working on this?

@darksylinc
Copy link
Contributor Author

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

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

@chapulina
Copy link
Contributor

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 chapulina removed the needs upstream release Blocked by a release of an upstream library label Aug 10, 2022
@chapulina chapulina changed the base branch from main to gz-sim7 August 10, 2022 18:21
@chapulina
Copy link
Contributor

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

@chapulina chapulina closed this Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🌱 garden Ignition Garden rendering Involves Ignition Rendering
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants