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 pose for TransformControl with GzPose #1642

Open
wants to merge 7 commits into
base: ign-gazebo3
Choose a base branch
from

Conversation

AzulRadio
Copy link
Contributor

Signed-off-by: youhy haoyuan2019@outlook.com

New feature

Summary

As title. The behavior should be exactly the same as before.

transform_control_replace_demo

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@AzulRadio AzulRadio added GUI Gazebo's graphical interface (not pure Ignition GUI) OOBE 📦✨ Out-of-box experience labels Aug 10, 2022
@osrf-triage osrf-triage added this to Inbox in Core development Aug 10, 2022
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Aug 10, 2022
Signed-off-by: youhy <haoyuan2019@outlook.com>
@AzulRadio AzulRadio force-pushed the azulradio/transform_control_gzpose branch from 73bb939 to 2f11a66 Compare August 10, 2022 22:07
@chapulina chapulina moved this from Inbox to In review in Core development Aug 10, 2022
@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #1642 (d71e224) into ign-gazebo3 (2a12514) will not change coverage.
The diff coverage is n/a.

❗ Current head d71e224 differs from pull request most recent head 54112d8. Consider uploading reports for the commit 54112d8 to get more accurate results

@@             Coverage Diff              @@
##           ign-gazebo3    #1642   +/-   ##
============================================
  Coverage        78.03%   78.03%           
============================================
  Files              255      255           
  Lines            15082    15082           
============================================
  Hits             11769    11769           
  Misses            3313     3313           

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

The sizing needs to be fixed, the values showing in the spin boxes are cut off
image

src/gui/plugins/transform_control/TransformControl.qml Outdated Show resolved Hide resolved
Signed-off-by: youhy <haoyuan2019@outlook.com>
Signed-off-by: youhy <haoyuan2019@outlook.com>
@AzulRadio AzulRadio added the needs upstream release Blocked by a release of an upstream library label Aug 17, 2022
@AzulRadio AzulRadio removed the needs upstream release Blocked by a release of an upstream library label Aug 18, 2022
@AzulRadio AzulRadio enabled auto-merge (squash) August 18, 2022 19:51
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

Nice work with allowing degrees in GzPose!

The top of each spin box is still cut off. I saw in 5fd186e the width was adjusted but it's most likely the height that needs to be updated.
image

Also, I think we should revert the width so that only 2 decimal places are showing (like how it was before) but this is only my preference/opinion. I'll leave it up to you to decide.

src/gui/plugins/transform_control/TransformControl.qml Outdated Show resolved Hide resolved
src/gui/plugins/transform_control/TransformControl.qml Outdated Show resolved Hide resolved
src/gui/plugins/transform_control/TransformControl.qml Outdated Show resolved Hide resolved
Signed-off-by: youhy <haoyuan2019@outlook.com>
Signed-off-by: youhy <haoyuan2019@outlook.com>
@AzulRadio
Copy link
Contributor Author

@jennuine The width and height both worked for me but didn't work for you. So there is no way I know what size would work for you and the only thing I can do is to increase the height and width a bit.

About the precision, I doubt if we can have 2 decimals without editing GzPose again. So let's just keep it 4 decimals for now.

@jennuine jennuine added the needs upstream release Blocked by a release of an upstream library label Aug 19, 2022
@jennuine
Copy link
Contributor

This is looking good @AzulRadio but requires a gz-gui3 release before it can be merged. It would be nice to get gazebosim/gz-gui#466 in before then so I'll take over this PR next week. Thanks for all your hard work! 🚀

@jennuine jennuine self-assigned this Aug 19, 2022
@jennuine jennuine disabled auto-merge August 25, 2022 17:32
@jennuine jennuine moved this from In review to To do in Core development Sep 2, 2022
@jennuine
Copy link
Contributor

jennuine commented Sep 2, 2022

Moving this to TODO to focus on the Garden release.

GzPose will need some additional updates so we don't have recession of functionality (eg., spinbox min/max values for each element)

@azeey azeey added beta Targeting beta release of upcoming collection and removed beta Targeting beta release of upcoming collection labels Jul 31, 2023
@azeey azeey added beta Targeting beta release of upcoming collection and removed beta Targeting beta release of upcoming collection labels Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel GUI Gazebo's graphical interface (not pure Ignition GUI) needs upstream release Blocked by a release of an upstream library OOBE 📦✨ Out-of-box experience
Projects
Status: To do
Development

Successfully merging this pull request may close these issues.

None yet

3 participants