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

Drag and drop mesh into scene #939

Merged
merged 11 commits into from
Aug 10, 2021
Merged

Conversation

jennuine
Copy link
Contributor

@jennuine jennuine commented Jul 28, 2021

🎉 New feature

Summary

Allows a user to drag and drop an OBJ, DAE, or STL mesh into the scene.

Peek 2021-07-21 13-41

Test it

  1. ign gazebo -v 4
  2. Drag a DAE or STL model and drop it into the scene

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

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine jennuine requested a review from chapulina as a code owner July 28, 2021 22:42
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jul 28, 2021
@jennuine jennuine mentioned this pull request Jul 28, 2021
8 tasks
@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #939 (0b8dcfb) into ign-gazebo3 (c57b3c9) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #939      +/-   ##
===============================================
+ Coverage        77.90%   77.96%   +0.05%     
===============================================
  Files              221      221              
  Lines            12678    12678              
===============================================
+ Hits              9877     9884       +7     
+ Misses            2801     2794       -7     
Impacted Files Coverage Δ
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 c57b3c9...0b8dcfb. Read the comment docs.

if (!common::EndsWith(lowerStr, ".dae")
&& !common::EndsWith(lowerStr, ".stl"))
{
ignwarn << "Only DAE and STL meshes are supported."
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should surface this as an actual dialog or warning. From a UX perspective, you would drag and drop into the GUI and then nothing would actually happen. If you didn't think to check the console log, it would be unintuitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh how I wish we had a proper snackbar gazebosim/gz-gui#44

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's this? 7717bac

Peek 2021-07-30 16-40

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to add an okay button to the dialog? It seems a bit unintuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/gui/plugins/scene3d/Scene3D.cc Outdated Show resolved Hide resolved
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
signals: void ErrorPopupTextChanged();

/// \brief Notify that an error has occurred (opens popup)
signals: void popupError();
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
signals: void popupError();
signals: void PopupError();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/gui/plugins/scene3d/Scene3D.cc Show resolved Hide resolved
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine jennuine requested a review from mjcarroll August 2, 2021 21:23
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@chapulina
Copy link
Contributor

Cross-referencing #185. This doesn't exactly match the feature on that issue, but it's close enough that we may consider that closed.

src/gui/plugins/scene3d/GzScene3D.qml Outdated Show resolved Hide resolved
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@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.

Works for me 👍 I just have some nitpicks on the error dialog.

Some ideas for the future, I think it would be nice to give the user these options before the model is spawned:

  1. A scale for the mesh before inserting
  2. Whether the model will be static

src/gui/plugins/scene3d/Scene3D.cc Outdated Show resolved Hide resolved
src/gui/plugins/scene3d/GzScene3D.qml Outdated Show resolved Hide resolved
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.

Thanks for iterating, LGTM

@chapulina chapulina enabled auto-merge (squash) August 10, 2021 18:56
@chapulina chapulina merged commit b7346a4 into ign-gazebo3 Aug 10, 2021
@chapulina chapulina deleted the jennuine/import_mesh_citadel branch August 10, 2021 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants