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

3D plot GUI plugin #917

Merged
merged 10 commits into from
Jul 29, 2021
Merged

3D plot GUI plugin #917

merged 10 commits into from
Jul 29, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Jul 15, 2021

🎉 New feature

Closes #231

Alternative to #332

Summary

Plots the path of entities in the 3D scene, parameters are customizable from SDF and / or the GUI.

WIP while I work on some final usability tweaks, and possibly a test.

Ready for review!

Test it

ign gazebo -r plot_3d.sdf

This is what it currently looks like:

plot3d

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 GUI Gazebo's graphical interface (not pure Ignition GUI) label Jul 15, 2021
@chapulina chapulina requested a review from mabelzhang July 15, 2021 19:50
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jul 15, 2021
@chapulina chapulina changed the title First iteration of the plot plugin, still need to work on usability 3D plot GUI plugin Jul 15, 2021
@chapulina chapulina mentioned this pull request Jul 15, 2021
@chapulina chapulina marked this pull request as ready for review July 15, 2021 23:56
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I made a initial review, I'm going a tried in my machine

<play_pause>true</play_pause>
<step>true</step>
<start_paused>true</start_paused>

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just curious, are you following any specific XML style for these suggestions? We haven't been enforcing any blank line policy so far.

examples/worlds/plot_3d.sdf Outdated Show resolved Hide resolved
include/ignition/gazebo/Util.hh Show resolved Hide resolved
src/gui/plugins/plot_3d/Plot3D_TEST.cc Outdated Show resolved Hide resolved
@caguero caguero self-assigned this Jul 19, 2021
@ahcorde
Copy link
Contributor

ahcorde commented Jul 22, 2021

Resize looks weird. X and R are bigger than the other parameters.

resize

src/gui/plugins/plot_3d/Plot3D_TEST.cc Outdated Show resolved Hide resolved
@chapulina chapulina mentioned this pull request Jul 23, 2021
12 tasks
@azeey azeey self-requested a review July 23, 2021 22:17
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Works great! Do you by any chance know why the color of the line changes over time? For example, in your video, the line for the lower arm starts out a light green color and goes to dark green.

include/ignition/gazebo/Util.hh Show resolved Hide resolved
include/ignition/gazebo/Util.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/Util.hh Outdated Show resolved Hide resolved
src/Util.cc Show resolved Hide resolved
src/Util.cc Outdated Show resolved Hide resolved
src/Util.cc Outdated Show resolved Hide resolved
src/gui/plugins/plot_3d/Plot3D.hh Outdated Show resolved Hide resolved
src/gui/plugins/plot_3d/Plot3D.cc Show resolved Hide resolved
src/gui/plugins/plot_3d/Plot3D.cc Outdated Show resolved Hide resolved
@chapulina
Copy link
Contributor Author

Thanks for the reviews, I believe I've addressed all comments ✔️

Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Thanks for iterating. There are a couple of codecheck issues, otherwise LGTM!

include/ignition/gazebo/Util.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/Util.hh Show resolved Hide resolved
@chapulina
Copy link
Contributor Author

Do you by any chance know why the color of the line changes over time?

Oh, btw, no, I don't. I think this may be an issue on the rendering end though. I'll see if I gather enough to ticket an issue.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina force-pushed the chapulina/6/plot3d branch from 8025da6 to 362570b Compare July 29, 2021 01:18
@chapulina chapulina requested a review from iche033 as a code owner July 29, 2021 01:18
@chapulina chapulina changed the base branch from main to ign-gazebo3 July 29, 2021 01:18
@chapulina chapulina added 🏰 citadel Ignition Citadel and removed 🏯 fortress Ignition Fortress labels Jul 29, 2021
@chapulina
Copy link
Contributor Author

Retargeted from Fortress to Citadel after iterating:

  1. Rebased from main to ign-gazebo3
  2. Updated target branch and label
  3. Made necessary changes in 362570b

If anyone wants to use this feature with Fortress before the PR is forward-ported, checkout this commit: 8025da6.

I'll merge this into Citadel if CI is happy.

@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #917 (2ba1658) into ign-gazebo3 (becec7e) will decrease coverage by 0.37%.
The diff coverage is 53.84%.

❗ Current head 2ba1658 differs from pull request most recent head 362570b. Consider uploading reports for the commit 362570b to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #917      +/-   ##
===============================================
- Coverage        78.16%   77.78%   -0.38%     
===============================================
  Files              218      219       +1     
  Lines            12372    12566     +194     
===============================================
+ Hits              9670     9774     +104     
- Misses            2702     2792      +90     
Impacted Files Coverage Δ
src/gui/plugins/plot_3d/Plot3D.cc 47.82% <47.82%> (ø)
src/Util.cc 92.96% <82.35%> (-2.22%) ⬇️

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 becec7e...362570b. Read the comment docs.

@chapulina chapulina merged commit eae6b9c into ign-gazebo3 Jul 29, 2021
@chapulina chapulina deleted the chapulina/6/plot3d branch July 29, 2021 17:58
@chapulina chapulina mentioned this pull request Jul 30, 2021
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants