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 Sensors aware of CameraPassCountPerGpuFlush & Scene::PostFrame #145

Merged
merged 4 commits into from
Jul 20, 2021

Conversation

darksylinc
Copy link
Contributor

🎉 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)

These changes makes sensors aware of legacy and new setting so that it adapts to both modes of operation.

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

Test it

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

Outside of tests, this PR will not change CameraPassCountPerGpuFlush (since it has to respect what the user selects); thus the default value of Ogre2ScenePrivate::cameraPassCountPerGpuFlush matters.

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
Requires ign-gazebo update to run correctly

Affects gazebosim/gz-rendering#323

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

iche033 commented Jul 19, 2021

@osrf-jenkins run tests please

@iche033
Copy link
Contributor

iche033 commented Jul 19, 2021

looks good to me. I just merged gazebosim/gz-rendering#353 so I think we'll need to wait a day for the ign-rendering nightly debs to be available first before getting the ubuntu CI build to pass.

@darksylinc
Copy link
Contributor Author

Mmm I wasn't ign-rendering expecting to be merged so fast! :)

I thought testing was going to be made "sandboxed" before merging.

Until this PR is not merged, ign-rendering's will break stuff for everybody. Please change Ogre2ScenePrivate::cameraPassCountPerGpuFlush default to 0 at least until ign-sensors is PR is merged.

@darksylinc
Copy link
Contributor Author

I can submit a PR if you are far away from a PC to submit that change yourself

@iche033
Copy link
Contributor

iche033 commented Jul 19, 2021

I tested the 3 PRs locally and it's working well. I was going to merge the 3 PRs in a row today but found that the CI uses nightlies instead of source builds so couldn't get this one in.

hmm ok I'll set Ogre2ScenePrivate::cameraPassCountPerGpuFlush to 0 and switch it back once the 3 PRs are in.

@iche033
Copy link
Contributor

iche033 commented Jul 19, 2021

hmm ok I'll set Ogre2ScenePrivate::cameraPassCountPerGpuFlush to 0 and switch it back once the 3 PRs are in.

gazebosim/gz-rendering#367

@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #145 (97766e6) into main (53c87ea) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
- Coverage   77.23%   77.22%   -0.02%     
==========================================
  Files          23       23              
  Lines        2355     2358       +3     
==========================================
+ Hits         1819     1821       +2     
- Misses        536      537       +1     
Impacted Files Coverage Δ
src/RenderingSensor.cc 93.93% <66.66%> (-2.73%) ⬇️

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 53c87ea...97766e6. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Jul 20, 2021

ubuntu build is passing, approving.

@iche033 iche033 merged commit d21c6f3 into gazebosim:main Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants