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

Insert Local Models #173

Merged
merged 50 commits into from
Aug 6, 2020
Merged

Insert Local Models #173

merged 50 commits into from
Aug 6, 2020

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Jun 3, 2020

This PR adds the functionality to insert local models similar to the currently existing shapes plugin. All the functionality is there, but some iterations will need to be made on the interface.

A few minor TODOs:

  • Refine insert model plugin UI
  • Integrate IGN_GAZEBO_RESOURCE_PATH (currently the path is hardcoded)
  • Some logic is searching for a forward slash in the path so some work will be needed here to be Windows compatible
  • Maybe a loading screen in placement mode before the visual model is placed into the scene? The larger models are taking awhile to display and it may confuse the user. - I'll leave this out for now, and we can address it later, I don't think it'd be a good use of time to investigate what might be a more complicated issue than it seems (I guess it boils down to how do we know a model has been visualized in the scene or not?)
  • Find a no thumbnail image to default to - I added one that I created, but we can swap in a better one if desired

John Shepherd added 6 commits June 1, 2020 16:05
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 requested a review from chapulina as a code owner June 3, 2020 19:48
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Jun 3, 2020
@codecov
Copy link

codecov bot commented Jun 3, 2020

Codecov Report

Merging #173 into ign-gazebo2 will increase coverage by 8.83%.
The diff coverage is 67.69%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo2     #173      +/-   ##
===============================================
+ Coverage        53.78%   62.61%   +8.83%     
===============================================
  Files              121      124       +3     
  Lines             5838     6209     +371     
===============================================
+ Hits              3140     3888     +748     
+ Misses            2698     2321     -377     
Impacted Files Coverage Δ
include/ignition/gazebo/Server.hh 100.00% <ø> (ø)
src/gui/Gui.cc 5.93% <0.00%> (+5.93%) ⬆️
src/gui/PathManager.cc 0.00% <0.00%> (ø)
src/systems/physics/Physics.cc 15.98% <ø> (ø)
src/systems/scene_broadcaster/SceneBroadcaster.cc 36.24% <0.00%> (-0.12%) ⬇️
src/systems/user_commands/UserCommands.hh 100.00% <ø> (ø)
src/systems/user_commands/UserCommands.cc 12.29% <16.66%> (ø)
src/Util.cc 84.74% <97.82%> (+8.35%) ⬆️
src/LevelManager.cc 90.71% <100.00%> (+49.10%) ⬆️
src/SdfGenerator.cc 95.61% <100.00%> (+0.07%) ⬆️
... and 27 more

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 2fd9547...b155ec0. Read the comment docs.

@JShep1 JShep1 added the close the gap Features from Gazebo-classic label Jun 3, 2020
@JShep1 JShep1 linked an issue Jun 3, 2020 that may be closed by this pull request
John Shepherd added 2 commits June 3, 2020 13:52
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@JShep1
Copy link
Author

JShep1 commented Jun 3, 2020

Proof of Concept:
localmodel

Obviously there's some GUI work to be done, I just didn't want to invest a huge amount of time into a direction we didn't necessarily want to go. I'll do more work after some discussion with @chapulina today.

John Shepherd and others added 5 commits June 3, 2020 18:47
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 did a first pass, let me know if you need help changing the QML

src/gui/plugins/insert_model/CMakeLists.txt Outdated Show resolved Hide resolved
src/gui/plugins/insert_model/InsertModel.cc Outdated Show resolved Hide resolved
src/gui/plugins/insert_model/InsertModel.cc Outdated Show resolved Hide resolved
src/gui/plugins/insert_model/InsertModel.cc Outdated Show resolved Hide resolved
src/gui/plugins/insert_model/InsertModel.hh Outdated Show resolved Hide resolved
src/gui/plugins/insert_model/InsertModel.hh Outdated Show resolved Hide resolved
src/gui/plugins/insert_model/InsertModel.cc Outdated Show resolved Hide resolved
src/gui/plugins/insert_model/InsertModel.cc Outdated Show resolved Hide resolved
import QtQuick.Layouts 1.3
import QtQuick.Controls.Styles 1.4

Rectangle {
Copy link
Contributor

@chapulina chapulina Jun 5, 2020

Choose a reason for hiding this comment

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

We've already talked about this in person, but I'll just leave some UI notes here so we don't forget:

  • All text should be the same size
  • Use Text.ElideRight for names
  • Support light / dark modes
  • Use Material.elevation to give each item a "card" look
  • Add more padding to the edges of the plugin, and between the items
  • Change the cursor to Qt.WaitCursor while the preview is being spawned

Copy link
Author

Choose a reason for hiding this comment

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

First four points are resolved in 47f214c

John Shepherd added 4 commits June 9, 2020 13:26
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@JShep1
Copy link
Author

JShep1 commented Jul 15, 2020

Updated preview:

localmodels

Copy link
Contributor

@nkoenig nkoenig left a comment

Choose a reason for hiding this comment

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

See #245 which has some suggestions.

@nkoenig
Copy link
Contributor

nkoenig commented Jul 15, 2020

I see no models when I run ign gazebo -v 4, and insert the "Resource Spawning". Is it possible to use the Fuel resource directory by default?

I also see no models when I run IGN_GAZEBO_RESOURCE_PATH=~/.ignition/fuel/fuel.ignitionrobotics.org/OpenRobotics/models ign gazebo -v 4.

Let me know what I'm doing wrong.

@JShep1
Copy link
Author

JShep1 commented Jul 15, 2020

See #245 which has some suggestions.

I'll take a look at these shortly

I also see no models when I run IGN_GAZEBO_RESOURCE_PATH=~/.ignition/fuel/fuel.ignitionrobotics.org/OpenRobotics/models ign gazebo -v 4.
Let me know what I'm doing wrong.

Currently, the resource finder looks for a model.config only one directory below the provided directory. Since fuel models follow a name like model-name-here/1/model.config (it would find something like model-name-here/model.config), the finder won't locate the model.config file. I could add a recursive approach that I had at an earlier version that handled these cases. I believe the reason we transitioned away from it was due to a discussion I had with Louise that indicated that ign-fuel-tools would handle this case. Any thoughts whether we should add the recursive approach? @chapulina @nkoenig I could be wrong, but I don't quite remember any bugs or issues with it

@chapulina
Copy link
Contributor

Is it possible to use the Fuel resource directory by default?

Yeah, John is right. Fuel's directory format doesn't match the expectations of the environment variable and should be treated as a separate case. (Note that this is also the reason why it's difficult to use Fuel with Gazebo-classic).

The current plan is to add Fuel as a separate item on the left panel, see the mockup. That will use ign-fuel-tools.

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
@nkoenig
Copy link
Contributor

nkoenig commented Jul 16, 2020

Is it possible to use the Fuel resource directory by default?

Yeah, John is right. Fuel's directory format doesn't match the expectations of the environment variable and should be treated as a separate case. (Note that this is also the reason why it's difficult to use Fuel with Gazebo-classic).

The current plan is to add Fuel as a separate item on the left panel, see the mockup. That will use ign-fuel-tools.

Thanks for the links. The Fuel use case is more important than the environment variable use case. The rataionale is that Fuel is the default, or should be the default, for Ignition while environment variables is more of an advanced feature. Also, SubT requires the Fuel use case. How about we get this version in, and start work on the Fuel use-case? I have some comments about the style and UX that I'll put in the issue.

@chapulina
Copy link
Contributor

How about we get this version in, and start work on the Fuel use-case?

Yup, @JShep1 is already on it.

The rataionale is that Fuel is the default, or should be the default

John, let's do the reverse of the mockup and put Fuel on top of the local directories then.


I'm also working on some UI tweaks for this PR.

* Some UI tweaks to Resource Spawner

Signed-off-by: Louise Poubel <[email protected]>

* adjust minimum width

Signed-off-by: Louise Poubel <[email protected]>

* folder icons

Signed-off-by: Louise Poubel <[email protected]>
Base automatically changed from chapulina/resource_path to ign-gazebo2 August 3, 2020 15:27
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
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.

LGTM. I just have a couple of trivial comments, and we should make sure CI comes back happy before merging.

src/gui/plugins/resource_spawner/ResourceSpawner.cc Outdated Show resolved Hide resolved
@chapulina chapulina merged commit 1539c12 into ign-gazebo2 Aug 6, 2020
@chapulina chapulina deleted the jshep1/local_models branch August 6, 2020 03:24
chapulina added a commit that referenced this pull request Aug 21, 2020
Signed-off-by: John Shepherd <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint close the gap Features from Gazebo-classic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Insert model from the GUI
3 participants