Added Interactive view control plugin #231

ahcorde · 2021-06-08T19:44:13Z

Signed-off-by: ahcorde ahcorde@gmail.com

🎉 New feature

Summary

This PR is part of the consolidation between the scene3d in ign-gui and ign-gazebo.

This plugin allows to control the camera in the 3D Scene.

Related PRs:

Test it

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

Can we add this plugin to scene3d.config?

This plugin allows to control the camera in the 3D Scene.

It's worth remembering that there are multiple ways of controlling the user camera, and they're being spread across various plugins:

ViewAngle provides canonical angles, set precise pose and projection type
The plugin added in Added camera tracking #226 provides "follow" and "move to"
This plugin allows orbiting with a mouse

In the future we may want to have more camera control functionality:

First person view control with mouse (I think it would be part of this plugin)
Keyboard based view control (probably also part of this plugin)

I can imagine this being confusing for users in the future.

I think this plugin is basic enough that it could be part of MinimalScene. On the other hand, some users may want to implement different orbit controls, so we either make this configurable, or let them implement their own thing. So since you've already started moving this to a new plugin, let's keep this separated.

I just think we should work on the names for all these plugins, so it's clear what each of them is for. This plugin is for orbiting with the mouse, possibly adding FPS control with the keyboard in the future. How about calling it InteractiveViewControl?

src/plugins/view_control/ViewControl.hh

src/plugins/scene3d/Scene3D.cc

chapulina · 2021-06-24T23:35:48Z

src/plugins/view_control/ViewControl.cc

+      return;
+
+    this->camera = std::dynamic_pointer_cast<rendering::Camera>(
+      this->scene->SensorByName("Scene3DCamera"));


I think that hardcoding the name of the camera may be a bit limiting. Can we assume that the first camera is the user camera? Like the screenshot plugin does:

https://github.com/ignitionrobotics/ign-gui/blob/d509ab956e50527daa6005345606b77c4f980bbf/src/plugins/screenshot/Screenshot.cc#L187-L198

With the recent changes in the PR of running in the process this may be not true. I included this change, I think all cameras except the one is moving should have a name similar to scene::camera(XXXX) otherwise should be named like model_name::link_name::camera_name

https://github.com/ignitionrobotics/ign-gui/blob/f3de5463e94e01ea9bebf640d1034be7356e6898/src/plugins/interactive_view_control/InteractiveViewControl.cc#L82-L95

With the recent changes in the PR of running in the process this may be not true

Yeah I see your point. The problem is that the scene of Scene3D or MinimalScene may not be called scene if the user passes a custom name through XML's <scene> element. I ran into this while writing a test for #226 which had a different scene name.

So I think we need a different way of identifying the user camera. On Gazebo classic there was a separate class for the user camera, so that was easy. On Ignition Rendering I don't think there's any distinction.

Another thing to keep in mind is that in the future we may want to restore support for multiple user cameras in the same scene, and these plugins that attach to a camera may want to choose which camera to attach to (like we had in ign-gui0).

Once gazebosim/gz-rendering#58 is tackled, maybe we could store a different property into the user camera(s)? @iche033 , do you have other ideas on how to identify if a camera is on the GUI or a sensor?

PR gazebosim/gz-rendering#358

src/plugins/view_control/ViewControl.cc

chapulina · 2021-06-24T23:38:26Z

include/ignition/gui/GuiEvents.hh

@@ -255,6 +255,24 @@ namespace ignition
        /// for this event.
        private: bool menuEnabled;
      };
+
+      class BlockOrbit : public QEvent


What's the intended use case for this event? I don't see it being used.

BlockOrbit is emitted when the transform mode is on. it used here https://github.com/ignitionrobotics/ign-gazebo/pull/854/files

chapulina · 2021-06-24T23:54:19Z

I should mention here that I updated #221 when #213 was merged forward. This is what's causing the conflicts here.

codecov · 2021-06-28T08:22:02Z

Codecov Report

Merging #231 (323471f) into main (973cbcc) will decrease coverage by 0.80%.
The diff coverage is 18.97%.

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
- Coverage   65.09%   64.29%   -0.81%     
==========================================
  Files          28       30       +2     
  Lines        4286     4386     +100     
==========================================
+ Hits         2790     2820      +30     
- Misses       1496     1566      +70

Impacted Files	Coverage Δ
include/ignition/gui/GuiEvents.hh	`100.00% <ø> (ø)`
...interactive_view_control/InteractiveViewControl.cc	`14.28% <14.28%> (ø)`
src/plugins/minimal_scene/MinimalScene.cc	`57.34% <18.18%> (+4.28%)`	⬆️
src/GuiEvents.cc	`100.00% <100.00%> (ø)`
...interactive_view_control/InteractiveViewControl.hh	`100.00% <100.00%> (ø)`
src/plugins/scene3d/Scene3D.cc	`48.89% <0.00%> (+0.26%)`	⬆️

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 973cbcc...323471f. Read the comment docs.

chapulina · 2021-07-01T00:45:45Z

How about calling it InteractiveViewControl?

Did you give this any thought, @ahcorde ?

ahcorde · 2021-07-01T10:33:17Z

How about calling it InteractiveViewControl?

Did you give this any thought, @ahcorde ?

Sorry I didn't see that comment. Done f3de546

chapulina

I did a first pass and pushed some trivial changes in 7dce9b0.

src/plugins/scene3d/Scene3D.cc

chapulina · 2021-07-06T22:42:26Z

src/plugins/minimal_scene/MinimalScene.cc

@@ -892,7 +838,8 @@ void RenderWindowItem::wheelEvent(QWheelEvent *_e)
 #endif
  double scroll = (_e->angleDelta().y() > 0) ? -1.0 : 1.0;
  this->dataPtr->renderThread->ignRenderer.NewMouseEvent(
-      this->dataPtr->mouseEvent, math::Vector2d(scroll, scroll));
+    this->dataPtr->mouseEvent);
+  this->dataPtr->mouseEvent.SetScroll(scroll, scroll);


With some manual testing I can see that this works, but I'm not sure why the SetScroll isn't before the event is sent.

Not really sure about this... uhmm

chapulina · 2021-07-06T22:55:02Z

src/plugins/interactive_view_control/InteractiveViewControl.cc

+  /// \brief
+  public: void OnRender();
+
+  math::Vector3d ScreenToScene(


We should consider putting this in a helper function that can be used by various plugins instead of copying it for every plugin that needs it. No need to do it now, maybe we can just add a TODO

where do you think this should live? ign-rendering ?

src/plugins/interactive_view_control/InteractiveViewControl.cc

include/ignition/gui/GuiEvents.hh

ahcorde · 2021-07-07T09:26:14Z

Requires gazebosim/gz-rendering#358

ahcorde · 2021-07-16T11:05:29Z

@osrf-jenkins retest this please

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-07-16T11:43:20Z

cleaned commit history because this PR was targeting a different branch

ahcorde · 2021-07-16T12:05:37Z

Windows warnings should be fixed with this PR #254

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

chapulina

LGTM! I pushed some docs and small tweaks in 323471f. Feel free to merge if those look ok to you, @ahcorde .

ahcorde mentioned this pull request Jun 9, 2021

New scene manager GUI plugin that works with ign-gui's MinimalScene gazebosim/gz-sim#813

Merged

18 tasks

chapulina added the 🏯 fortress Ignition Fortress label Jun 17, 2021

This was referenced Jun 18, 2021

Consolidate Scene3D with ign-gazebo's GzScene3D #137

Closed

Added Mouse Events #228

Merged

Split transport scene manager into a plugin outside Scene3D #221

Merged

ahcorde changed the title ~~Added view control plugin~~ Added camera control plugin Jun 22, 2021

ahcorde marked this pull request as ready for review June 24, 2021 22:00

ahcorde requested a review from chapulina as a code owner June 24, 2021 22:00

chapulina reviewed Jun 24, 2021

View reviewed changes

ahcorde requested a review from jennuine as a code owner June 25, 2021 12:24

ahcorde requested a review from chapulina July 1, 2021 11:02

ahcorde changed the title ~~Added camera control plugin~~ Added Interactive view control plugin Jul 1, 2021

Base automatically changed from chapulina/6/new_scene to main July 1, 2021 18:23

chapulina mentioned this pull request Jul 2, 2021

Added camera tracking #226

Merged

8 tasks

ahcorde mentioned this pull request Jul 2, 2021

UserData methods moved from Visual to Node gazebosim/gz-rendering#358

Merged

7 tasks

chapulina reviewed Jul 6, 2021

View reviewed changes

Added Interactive view control plugin

18d35ba

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde force-pushed the ahcorde/plugin/view_control branch from c81302d to 18d35ba Compare July 16, 2021 11:42

ahcorde added 2 commits July 16, 2021 16:56

Merge branch 'main' into ahcorde/plugin/view_control

2132e2c

Merge branch 'main' into ahcorde/plugin/view_control

6eef057

Merge branch 'main' into ahcorde/plugin/view_control

1f01c22

ahcorde requested a review from chapulina July 20, 2021 10:44

Documentation and small tweaks

323471f

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

chapulina approved these changes Jul 20, 2021

View reviewed changes

ahcorde enabled auto-merge (squash) July 20, 2021 20:57

chapulina disabled auto-merge July 20, 2021 22:03

chapulina merged commit d57e501 into main Jul 20, 2021

chapulina deleted the ahcorde/plugin/view_control branch July 20, 2021 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Interactive view control plugin #231

Added Interactive view control plugin #231

ahcorde commented Jun 8, 2021

chapulina left a comment

chapulina Jun 24, 2021

ahcorde Jul 1, 2021

chapulina Jul 2, 2021

ahcorde Jul 2, 2021

chapulina Jun 24, 2021

ahcorde Jun 28, 2021

chapulina commented Jun 24, 2021

codecov bot commented Jun 28, 2021 •

edited

Loading

chapulina commented Jul 1, 2021

ahcorde commented Jul 1, 2021

chapulina left a comment

chapulina Jul 6, 2021

ahcorde Jul 7, 2021

chapulina Jul 6, 2021

ahcorde Jul 7, 2021

ahcorde commented Jul 7, 2021

ahcorde commented Jul 16, 2021

ahcorde commented Jul 16, 2021

ahcorde commented Jul 16, 2021

chapulina left a comment

Added Interactive view control plugin #231

Added Interactive view control plugin #231

Conversation

ahcorde commented Jun 8, 2021

🎉 New feature

Summary

Test it

Checklist

chapulina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chapulina commented Jun 24, 2021

codecov bot commented Jun 28, 2021 • edited Loading

Codecov Report

chapulina commented Jul 1, 2021

ahcorde commented Jul 1, 2021

chapulina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcorde commented Jul 7, 2021

ahcorde commented Jul 16, 2021

ahcorde commented Jul 16, 2021

ahcorde commented Jul 16, 2021

chapulina left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 28, 2021 •

edited

Loading