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

Replace renderOneFrame for per-workspace update calls #353

Merged
merged 22 commits into from
Jul 19, 2021

Conversation

darksylinc
Copy link
Contributor

🎉 New feature

Implements #323 for ign-rendering

Summary

Explanation is in the ticket

The ticket shouldn't be closed until all modules are merged.

Test it

This PR is currently defaulting SetCameraPassCountPerGpuFlush to 0 avoid breaking ign-sensors.

In order to fully test it, checkout:

  1. https://github.com/darksylinc/ign-rendering/tree/matias-PostFrame5
  2. https://github.com/darksylinc/ign-gazebo/tree/matias-PostFrame5
  3. https://github.com/darksylinc/ign-sensors/tree/matias-PostFrame5

And set SetCameraPassCountPerGpuFlush to non-0 (e.g. set Ogre2ScenePrivate::cameraPassCountPerGpuFlush to 6)

This change is mostly a performance one; but it also affects particle FXs (and possibly some postprocessing FXs) because without this PR, Ogre2GpuRays rendering a cubemap with particle FXs will render each of the 6 faces in a different time; i.e. as if the simulation moved forward 6 times

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

@darksylinc darksylinc requested a review from iche033 as a code owner June 26, 2021 21:53
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jun 26, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Jun 26, 2021
@codecov
Copy link

codecov bot commented Jun 26, 2021

Codecov Report

Merging #353 (2720eb8) into main (f3f75a5) will increase coverage by 0.17%.
The diff coverage is 86.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
+ Coverage   58.02%   58.20%   +0.17%     
==========================================
  Files         170      170              
  Lines       16675    16787     +112     
==========================================
+ Hits         9676     9771      +95     
- Misses       6999     7016      +17     
Impacted Files Coverage Δ
include/ignition/rendering/Camera.hh 100.00% <ø> (ø)
include/ignition/rendering/Scene.hh 0.00% <ø> (ø)
include/ignition/rendering/base/BaseScene.hh 0.00% <ø> (ø)
src/base/BaseScene.cc 74.80% <0.00%> (-0.94%) ⬇️
ogre2/src/Ogre2Scene.cc 81.39% <86.11%> (+0.65%) ⬆️
include/ignition/rendering/base/BaseCamera.hh 67.04% <100.00%> (+0.24%) ⬆️
ogre2/src/Ogre2DepthCamera.cc 91.07% <100.00%> (+0.09%) ⬆️
ogre2/src/Ogre2GpuRays.cc 95.03% <100.00%> (+0.09%) ⬆️
ogre2/src/Ogre2RenderTarget.cc 81.47% <100.00%> (+0.22%) ⬆️
ogre2/src/Ogre2SelectionBuffer.cc 80.59% <100.00%> (+0.90%) ⬆️
... and 2 more

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 f3f75a5...2720eb8. Read the comment docs.

@chapulina chapulina moved this from Inbox to In review in Core development Jun 28, 2021
}

//////////////////////////////////////////////////
void BaseScene::SetCameraPassCountPerGpuFlush(uint8_t _numPass)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
void BaseScene::SetCameraPassCountPerGpuFlush(uint8_t _numPass)
void BaseScene::SetCameraPassCountPerGpuFlush(uint8_t /*_numPass*/)

ogre2/src/Ogre2ThermalCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2SelectionBuffer.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2Scene.cc Outdated Show resolved Hide resolved
auto ogreRoot = engine->OgreRoot();
Ogre::CompositorManager2 *ogreCompMgr = ogreRoot->getCompositorManager2();

// engine->OgreRoot()->renderOneFrame();
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to better document this.

That line is very useful, specially if debugging "what should happen when you let Ogre do anything" vs "what happens when we do some lower level calls ourselves"

ogre2/src/Ogre2GpuRays.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2GpuRays.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2DepthCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2DepthCamera.cc Outdated Show resolved Hide resolved
include/ignition/rendering/base/BaseCamera.hh Outdated Show resolved Hide resolved
@chapulina
Copy link
Contributor

Hi @darksylinc , mind updating the PR title to be a bit more descriptive? Thanks!

@darksylinc darksylinc changed the title Matias post frame5 Replace renderOneFrame for per-workspace update calls Jul 4, 2021
@darksylinc
Copy link
Contributor Author

Hi @darksylinc , mind updating the PR title to be a bit more descriptive? Thanks!

Done! Sorry!!! 😦

ogre2/include/ignition/rendering/ogre2/Ogre2Scene.hh Outdated Show resolved Hide resolved
include/ignition/rendering/Scene.hh Outdated Show resolved Hide resolved
Migration.md Outdated Show resolved Hide resolved
include/ignition/rendering/base/BaseScene.hh Outdated Show resolved Hide resolved
ogre2/src/Ogre2Scene.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2Scene.cc Outdated Show resolved Hide resolved
@iche033
Copy link
Contributor

iche033 commented Jul 12, 2021

This looks good. Can you also update your other branches (ign-sensors and ign-gazebo) to rebase off of main and create PRs for those (based on discussion here)? thanks

chapulina and others added 18 commits July 17, 2021 19:03
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Add Scene::PostRenderGpuFlush

Breaks ABI.
Needs changes to other components as well, so they call
Scene::PostRenderGpuFlush

Affects gazebosim#323

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Affects gazebosim#323

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Renamed PostRenderGpuFlush to PostRender
PostRenderGpuFlush was leaking too much internal behavior of how the
engine works into the user.

The new way of forcing the user to pair PreRender and PostRender calls
is much easier to understand and user-friendly

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Fixes GpuRays/GpuRaysTest.RaysParticles failure
Particle FXs should now be working as intended

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
SetNumCameraPassesPerGpuFlush -> SetCameraPassCountPerGpuFlush
GetNumCameraPassesPerGpuFlush -> GetCameraPassCountPerGpuFlush
GetLegacyAutoGpuFlush -> LegacyAutoGpuFlush

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
INTEGRATION_depth_camera now succeeds

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Fix wrong documentation about currNumCameraPasses

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Better document code in FlushGpuCommandsOnly

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
- Do not recommend the user to call Camera::PreRender in docs. This is
done implicitly via Scene::PreRender
- Rename GetCameraPassCountPerGpuFlush -> CameraPassCountPerGpuFlush
- CameraPassCountPerGpuFlush was incorrectly always returning 0
- Grammar mistakes in comments

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
@iche033 iche033 dismissed ahcorde’s stale review July 19, 2021 22:06

I'll address style issues in a separate PR

@iche033 iche033 merged commit b7d7d63 into gazebosim:main Jul 19, 2021
Core development automation moved this from In review to Done Jul 19, 2021
WilliamLewww pushed a commit to WilliamLewww/ign-rendering that referenced this pull request Dec 7, 2021
* 🎈 5.1.0 (gazebosim#351)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Replace renderOneFrame for per-workspace update calls

Add Scene::PostRenderGpuFlush

Breaks ABI.
Needs changes to other components as well, so they call
Scene::PostRenderGpuFlush

Affects gazebosim#323

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Add Scene::SetLegacyAutoGpuFlush

Affects gazebosim#323

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Add Scene::SetNumCameraPassesPerGpuFlush

Renamed PostRenderGpuFlush to PostRender
PostRenderGpuFlush was leaking too much internal behavior of how the
engine works into the user.

The new way of forcing the user to pair PreRender and PostRender calls
is much easier to understand and user-friendly

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Assert when PreRender/PostRender calls are used incorrectly

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Fix warnings generated by gazebo in legacy mode

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Add missing listener triggers

Fixes GpuRays/GpuRaysTest.RaysParticles failure
Particle FXs should now be working as intended

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Rename functions to be consistent w/ Ignition naming convention

SetNumCameraPassesPerGpuFlush -> SetCameraPassCountPerGpuFlush
GetNumCameraPassesPerGpuFlush -> GetCameraPassCountPerGpuFlush
GetLegacyAutoGpuFlush -> LegacyAutoGpuFlush

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Document that SetCameraPassCountPerGpuFlush is an upper bound

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Document Camera::Update shouldn't be called if there's many cameras

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Check in Render() call that we're inside PreRender / PostRender

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Fix INTEGRATION_camera crash

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Fix EndFrame not getting called when in non-legacy mode

INTEGRATION_depth_camera now succeeds

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Fix wrong documentation about Camera::Capture

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Update Migration notes w/ Scene::PostRender

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Add missing call in doc's recomendations

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Improve documentation and make asserts more helpful

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Default cameraPassCountPerGpuFlush until all modules are merged

Fix wrong documentation about currNumCameraPasses

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Remove unnecessary headers

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Style changes for consistency with the rest of the codebase

Better document code in FlushGpuCommandsOnly

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Multiple fixes

- Do not recommend the user to call Camera::PreRender in docs. This is
done implicitly via Scene::PreRender
- Rename GetCameraPassCountPerGpuFlush -> CameraPassCountPerGpuFlush
- CameraPassCountPerGpuFlush was incorrectly always returning 0
- Grammar mistakes in comments

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Make cameraPassCountPerGpuFlush default to 6

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
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.

None yet

4 participants