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

Split transport scene manager into a plugin outside Scene3D #221

Merged
merged 23 commits into from
Jul 1, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented May 8, 2021

🎉 New feature

Part of #137 , alternative to #217

Needs gazebosim/gz-rendering#320

Summary

This is a step towards reducing duplication between 3D scenes. Initially, @ahcorde and I had talked about taking an approach similar to the one used by plotting. That is providing an interface that is wrapped by an ign-gui plugin, and also wrapped by an ign-gazebo plugin, which adds Gazebo-specific functionality on top. @ahcorde started working on this approach in #217, and I also iterated a bit on top of it on 2fc70f2. But while at it, I realized that this approach may not be the best fit for Scene3D, because there's actually a simpler way.

My new idea is to leverage the Render event to perform all scene update tasks such as adding, removing and updating visuals. The Scene3D plugin itself should be very minimal, just focusing on running the render loop and providing some orbit controls. I'm leaving this minimal functionality on the new MinimalScene plugin, while all other functionality should be pushed to new plugins, like the TransportSceneManager that's being proposed here.

This approach allows us to keep Ignition GUI's Scene3D almost the same and add all new functionality on top. I think we could do the same with ignition::gazebo::Scene3D, see gazebosim/gz-sim#813. That is:

  • copy all the logic that is not already in ignition::gui::Scene3D to a new plugin, GazeboSceneManager
  • GazeboSceneManager will:
    • not work by itself. Like TransportSceneManager, it must be loaded alongside ignition::gui::MinimalScene
    • have no code that paints to the window (like texture IDs, instantiating a rendering thread, etc). That's the gui::MinimalScene's responsibility.
    • install a filter to the Render event and perform all rendering calls within that callback.
  • Because we've been actively developing it, gazebo::Scene3D may have some functions and bugfixes that we'll need to port to gui::MinimalScene. We just need to be careful to push too much functionality into gui::MinimalScene. Let's keep that plugin lean. Any well scoped feature should go into a separate plugin that ties into the render event.

Test it

Try the new scene_provider example, see instructions on README added.

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

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label May 8, 2021
@chapulina chapulina requested a review from ahcorde May 8, 2021 01:40
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label May 8, 2021
Signed-off-by: Louise Poubel <[email protected]>
@ahcorde ahcorde mentioned this pull request May 26, 2021
8 tasks
@ahcorde ahcorde mentioned this pull request Jun 8, 2021
8 tasks
* Add sky tag to the new Scene3D

Signed-off-by: ahcorde <[email protected]>

* Update docs

Signed-off-by: ahcorde <[email protected]>
@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #221 (9b2cea1) into main (a9ddfad) will decrease coverage by 1.92%.
The diff coverage is 52.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #221      +/-   ##
==========================================
- Coverage   67.08%   65.15%   -1.93%     
==========================================
  Files          25       26       +1     
  Lines        3138     3811     +673     
==========================================
+ Hits         2105     2483     +378     
- Misses       1033     1328     +295     
Impacted Files Coverage Δ
src/plugins/minimal_scene/MinimalScene.cc 52.15% <52.15%> (ø)
...s/transport_scene_manager/TransportSceneManager.cc 52.29% <52.29%> (ø)
src/plugins/scene3d/Scene3D.cc 48.89% <100.00%> (+0.46%) ⬆️
src/plugins/grid_3d/Grid3D.cc

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 a9ddfad...9b2cea1. Read the comment docs.

@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Jun 14, 2021
@chapulina
Copy link
Contributor Author

chapulina commented Jun 17, 2021

Updates

  1. Moved the new approach to a new plugin, MinimalScene, so we can tick-tock the change for users. Scene3D is deprecated on v6 and will be removed on v7.
  2. Improved the example so the box moves:

random_box

TODO:

  • Add tests

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

@ahcorde , I'm opening this PR for review. I've added tests, a tutorial, and a migration path.

@chapulina chapulina marked this pull request as ready for review June 19, 2021 02:34
src/plugins/minimal_scene/MinimalScene.cc Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
public: common::MouseEvent mouseEvent;

/// \brief Mouse move distance since last event.
public: math::Vector2d drag;
Copy link
Contributor

Choose a reason for hiding this comment

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

I added a plugin to move the camera with mouse #231, this will remove some code here. should we merge this first?

src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
test/integration/minimal_scene.cc Show resolved Hide resolved
test/integration/transport_scene_manager.cc Show resolved Hide resolved
@ahcorde ahcorde mentioned this pull request Jun 22, 2021
4 tasks
@chapulina chapulina requested a review from jennuine as a code owner June 24, 2021 22:58
Signed-off-by: Louise Poubel <[email protected]>
src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.hh Show resolved Hide resolved
test/integration/minimal_scene.cc Show resolved Hide resolved
test/integration/minimal_scene.cc Show resolved Hide resolved
test/integration/minimal_scene.cc Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.hh Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.hh Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.hh Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina merged commit 6f50c9c into main Jul 1, 2021
@chapulina chapulina deleted the chapulina/6/new_scene branch July 1, 2021 18:23
@ahcorde
Copy link
Contributor

ahcorde commented Jul 1, 2021

@chapulina, these two PRs are the following ones:

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