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

Make Gazebo aware of SetCameraPassCountPerGpuFlush #921

Merged
merged 6 commits into from
Jul 22, 2021

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Jul 17, 2021

🎉 New feature

See gazebosim/gz-rendering/issues/323

Summary

ign-rendering has a new feature aimed to improve performance but also fixes a few bugs related to particle simulations; which is controlled via Scene::SetCameraPassCountPerGpuFlush.

A value of 0 will use the legacy mode (i.e. previous behavior) to ease porting from older version.
A value > 0 controls performance vs RAM tradeoff (note that it is an upper limit; a very high value doesn't immediately waste RAM. See Scene::SetCameraPassCountPerGpuFlush documentation for details)

This PR depends on gazebosim/gz-rendering#353 otherwise it will not compile.

Test it

Just run the code again. This PR changes SetCameraPassCountPerGpuFlush explicitly to 6 to take advantage of the new feature. Set it to 0 for legacy.

As for performance, there won't be yet measurable differences because most sensors are currently downloading rendering data immediately after submitting rendering; which forces a flush per sensor. The final improvement will come when we make use of AsyncTickets in Ogre 2.2

However after the PR it can be immediately tested that particles FXs involving GpuRays behave differently, because pre-PR each cubemap face would be rendered at different timestamps; post-PR they are all rendered at the same timestamp (even in legacy mode).

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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

Requires ign-rendering update to compile correctly

Affects gazebosim/gz-rendering#323

Signed-off-by: Matias N. Goldberg <[email protected]>
Signed-off-by: Matias N. Goldberg <[email protected]>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

style

src/gui/plugins/scene3d/Scene3D.cc Outdated Show resolved Hide resolved
src/systems/sensors/Sensors.cc Outdated Show resolved Hide resolved
Signed-off-by: Matias N. Goldberg <[email protected]>
@iche033
Copy link
Contributor

iche033 commented Jul 21, 2021

@osrf-jenkins run tests please

@iche033 iche033 requested a review from ahcorde July 21, 2021 21:02
@iche033 iche033 merged commit b08d63c into gazebosim:main Jul 22, 2021
WilliamLewww pushed a commit to WilliamLewww/ign-gazebo that referenced this pull request Dec 7, 2021
* Call Scene::PostRenderGpuFlush

Requires ign-rendering update to compile correctly

Affects gazebosim/gz-rendering#323

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

* Update to use the renamed PostRender call

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

* Avoid warnings from Legacy mode

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

* Default to SetNumCameraPassesPerGpuFlush = 6

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

* Rename SetNumCameraPassesPerGpuFlush -> SetCameraPassCountPerGpuFlush

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

* Style changes

Signed-off-by: Matias N. Goldberg <[email protected]>
Signed-off-by: William Lew <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants