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

Send mouse hover event only on spawn preview to improve framerate #950

Closed
wants to merge 1 commit into from

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Jul 30, 2021

Signed-off-by: Ian Chen [email protected]

🦟 Bug fix

Summary

Scene3D appear to be slow when there are high poly count meshes in the scene. This turns out that it's not a rendering issue. The problem is that we are making ray queries every frame through the ScreenToScene call in the BroadcastHoverPos function. I believe this is intended mainly for spawning entities. The hoverDirty variable is never set to false even when the mouse is outside the 3D window. It's only set to false in spawn entity mode. So it's incurring unnecessary computation every frame.

Note with this change, we will only broadcast the hover event on spawn entity mode. The gain here is a big improvement in frame rate.

To reproduce the issue:

launch ign-gazebo and drag and drop a high polycount model into the scene, e.g. Urban 2 Story model. Try moving the camera and notice that it's laggy.

Before this change, I get ~3-4 FPS, and after this change, I get 40-50 FPS

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

@iche033 iche033 requested a review from chapulina as a code owner July 30, 2021 23:31
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jul 30, 2021
@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #950 (27d104a) into ign-gazebo3 (59a2faf) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gazebo3     #950   +/-   ##
============================================
  Coverage        77.80%   77.80%           
============================================
  Files              220      220           
  Lines            12579    12579           
============================================
  Hits              9787     9787           
  Misses            2792     2792           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59a2faf...27d104a. Read the comment docs.

@@ -889,7 +889,7 @@ void IgnRenderer::HandleMouseEvent()
/////////////////////////////////////////////////
void IgnRenderer::BroadcastHoverPos()
{
if (this->dataPtr->hoverDirty)
if (this->dataPtr->spawnPreview && this->dataPtr->hoverDirty)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's a performance issue, but we need to find an alternative solution. Maybe throttle it?

@chapulina chapulina added performance Runtime performance GUI Gazebo's graphical interface (not pure Ignition GUI) labels Aug 4, 2021
@iche033 iche033 closed this Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel GUI Gazebo's graphical interface (not pure Ignition GUI) performance Runtime performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants