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

Added transform control and select entities GUI plugins #854

Merged
merged 41 commits into from
Aug 14, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jun 9, 2021

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

🎉 New feature

Summary

This PR is part of the consolidation between Scene 3D in ign-gazebo and ign-gui. This PR adds two independent plugin to select entities and transform control.

It builds on top of #813

Test it

ign gazebo --gui-config src/ign-gazebo/src/gui/gui.config rolling_shapes.sdf -v 4

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 and others added 3 commits May 12, 2021 14:08
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
ahcorde and others added 4 commits June 18, 2021 15:01
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from chapulina June 22, 2021 17:13
ahcorde and others added 4 commits June 22, 2021 19:14
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I wasn't able to compile this branch because I think I have the wrong combination of ign-gui branches.

The main high-level comment I have is whether it would be better to combine these 2 with the existing TransformControl plugin. I think all this logic should originally have been contained in that plugin, but we didn't have a mechanism for manipulating the 3D scene, so part of it ended up in Scene3D.

…ahcorde/plugin/selectEntities_transformControl
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina chapulina added the 🏯 fortress Ignition Fortress label Aug 4, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
…robotics/ign-gazebo into chapulina/6/scene_manager

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@ahcorde ahcorde self-assigned this Aug 10, 2021
@chapulina
Copy link
Contributor

@ahcorde . I started reviewing this and making some changes in ahcorde/plugin/selectEntities_transformControl...chapulina/select_transform. I thought they would be mainly trivial changes, updating style, namespaces and whatnot, as well as combining TransformControl with TransformControlLogic. But then I realized that some features are not working yet on for transform controls:

  • Snapping (the keyEvent never has modifiers like Control, so it's not possible to snap while dragging)
  • In select mode, select an entity, then change to translate mode. The transform controls should attach to the last selected entity, but it doesn't attach to anything.

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 11, 2021

Requires gazebosim/gz-gui#268

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Aug 11, 2021
ahcorde and others added 3 commits August 12, 2021 12:34
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I pushed some documentation and style updates in caf08a9.

I noticed a change in behaviour. Pressing Shift is toggling between world and local frames, but the correct behaviour is to use the world frame while the Shift key is being held, and going back to local when the key is released, as explained in the tutorial:

https://ignitionrobotics.org/docs/edifice/manipulating_models#align-to-world-frame

src/gui/plugins/transform_control/TransformControl.cc Outdated Show resolved Hide resolved
src/gui/plugins/scene_manager/GzSceneManager.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 13, 2021

I noticed a change in behaviour. Pressing Shift is toggling between world and local frames, but the correct behaviour is to use the world frame while the Shift key is being held, and going back to local when the key is released, as explained in the tutorial:

I was using the old KeyEvent. Solved e6e3e16

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde changed the title Added tranform control and select entities GUI plugins Added transform control and select entities GUI plugins Aug 13, 2021
@chapulina
Copy link
Contributor

Everything seems to be working well for me now, thanks! I just have this final question: e6e3e16#commitcomment-54864874

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

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Works for me! We just need some fresh ign-gui nightlies to go with this. I'm on it.

Triggered: https://build.osrfoundation.org/job/ign-gui6-debbuilder/

@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #854 (943679d) into main (eaf7aab) will decrease coverage by 0.90%.
The diff coverage is 4.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #854      +/-   ##
==========================================
- Coverage   65.43%   64.53%   -0.91%     
==========================================
  Files         246      246              
  Lines       19433    19729     +296     
==========================================
+ Hits        12716    12732      +16     
- Misses       6717     6997     +280     
Impacted Files Coverage Δ
include/ignition/gazebo/gui/GuiEvents.hh 0.00% <0.00%> (ø)
src/gui/plugins/scene3d/Scene3D.cc 10.79% <0.00%> (-0.01%) ⬇️
src/rendering/SceneManager.cc 35.13% <ø> (-0.07%) ⬇️
.../gui/plugins/transform_control/TransformControl.cc 7.23% <4.45%> (-11.92%) ⬇️
src/gui/plugins/plot_3d/Plot3D.cc 47.82% <0.00%> (+4.34%) ⬆️

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 eaf7aab...943679d. Read the comment docs.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
…//github.com/ignitionrobotics/ign-gazebo into ahcorde/plugin/selectEntities_transformControl

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Aug 13, 2021
@chapulina chapulina merged commit fc35de3 into main Aug 14, 2021
@chapulina chapulina deleted the ahcorde/plugin/selectEntities_transformControl branch August 14, 2021 00:49
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