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 origin axes config plugin #150

Closed
wants to merge 22 commits into from

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 21, 2020

I have been playing a little bit around ignition-gazebo. I have created a config plugin to add and configure the origin axes. I was trying to do something similar as the grid. This is created in ignition-rendering using the method showGrid(). I was planning to add a new method called showOriginAxes(). but arrows are build based on two visuals: a cone and a cylinder. I was not able to get the arrows in the config plugins based only in the ID. is there any way to get the arrows based on the name?

Origin_axes_plugin_config

Signed-off-by: ahcorde [email protected]

Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde added the enhancement New feature or request label May 21, 2020
@ahcorde ahcorde requested a review from chapulina as a code owner May 21, 2020 08:53
@ahcorde ahcorde self-assigned this May 21, 2020
@github-actions github-actions bot added the 🔮 dome Ignition Dome label May 21, 2020
@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #150 (fd1b4e7) into ign-gazebo4 (302f5ed) will decrease coverage by 0.06%.
The diff coverage is 76.64%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo4     #150      +/-   ##
===============================================
- Coverage        77.37%   77.31%   -0.07%     
===============================================
  Files              217      219       +2     
  Lines            12217    12567     +350     
===============================================
+ Hits              9453     9716     +263     
- Misses            2764     2851      +87     
Impacted Files Coverage Δ
src/systems/joint_controller/JointController.cc 77.17% <12.50%> (-6.16%) ⬇️
...int_position_controller/JointPositionController.cc 75.49% <50.00%> (-0.51%) ⬇️
src/gui/AboutDialogHandler.cc 66.66% <66.66%> (ø)
...trajectory_controller/JointTrajectoryController.cc 78.20% <78.20%> (ø)
src/gui/GuiRunner.cc 55.93% <83.33%> (+1.38%) ⬆️
src/gui/Gui.cc 65.90% <100.00%> (ø)
...trajectory_controller/JointTrajectoryController.hh 100.00% <100.00%> (ø)
.../plugins/component_inspector/ComponentInspector.cc 7.22% <0.00%> (-1.39%) ⬇️
src/EntityComponentManager.cc 85.86% <0.00%> (-0.18%) ⬇️
... and 3 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 5e0c773...7afe270. Read the comment docs.

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 the new feature 😄 I'm not sure why, but it failed to create the arrows for me, printed these and then crashed when I tried to change the size:

[GUI] [Wrn] [OriginAxesConfig.cc:161] Creating origin axes                                                                             
[GUI] [Err] [OriginAxesConfig.cc:169] Failed to create origin axes, origin axes config plugin won't work.
/home/chapulina/dome_ws/install/lib/ign-gazebo-4/plugins/gui/libOriginAxesConfig.so(_ZN8ignition6gazebo16OriginAxesConfig10UpdateSizeEi+0x31) [0x7f9128df05e1] /home/chapulina/dome_ws/src/ign-gazebo/src/gui/plugins/origin_axes/OriginAxesConfig.cc:198
/home/chapulina/dome_ws/install/lib/ign-gazebo-4/plugins/gui/libOriginAxesConfig.so(_ZN8ignition6gazebo16OriginAxesConfig11qt_metacallEN11QMetaObject4CallEiPPv+0x63) [0x7f9128df4563] /home/chapulina/dome_ws/build/ignition-gazebo4/src/gui/plugins/origin_axes/moc_OriginAxesConfig.cpp:110

I was not able to get the arrows in the config plugins based only in the ID. is there any way to get the arrows based on the name?

Looking at the implementation, it looks like each arrow is a visual composed of 2 child visuals (cone and cylinder). So I think you should be able to give it a name when you create it, and access it later using Scene::VisualByName. Just be careful with overlapping names in case the user instantiates more than one origin axis.


Going forward, we should target plugins like this which don't* have any Gazebo-specific code to ign-gui. This way they're available to a wider user base. No need to change this PR, just something to keep in mind.

* I know that right now we only have the render event on this repo, but we should move that to ign-gui soon.

src/gui/plugins/origin_axes/OriginAxesConfig.cc Outdated Show resolved Hide resolved
src/gui/plugins/origin_axes/OriginAxesConfig.cc Outdated Show resolved Hide resolved
src/gui/plugins/origin_axes/OriginAxesConfig.cc Outdated Show resolved Hide resolved
src/gui/plugins/origin_axes/OriginAxesConfig.cc Outdated Show resolved Hide resolved
src/gui/plugins/origin_axes/OriginAxesConfig.cc Outdated Show resolved Hide resolved
src/gui/plugins/origin_axes/OriginAxesConfig.cc Outdated Show resolved Hide resolved
auto root = scene->RootVisual();
this->dataPtr->x_axis = scene->CreateArrowVisual();
this->dataPtr->y_axis = scene->CreateArrowVisual();
this->dataPtr->z_axis = scene->CreateArrowVisual();
Copy link
Contributor

Choose a reason for hiding this comment

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

On Gazebo-classic the origin axes were implemented using dynamic lines instead of arrows. Most of the time users need the origin to be long and without arrows, see this comment. @iche033 can correct me if I'm wrong, but I believe the lines should be less expensive to render, since we don't have the arrow head and the extra triangles from the cylinders. I'm not advocating for a change, I just think it should be pointed out.


Having said all that, I think it's totally valid to add support for custom axes like RViz's axes displays, which ties into the work @Sarath18 will be doing 😄 To that end, here are some suggestions to make the current plugin more flexible:

  • Add an option to set the pose
  • Add an option to set the radius
  • Add an option to change the arrows into lines

This is definitely feature creep for this PR, but I'll just mention it as an idea for the future:

  • Let the user attach an axis to any entity that has a pose component

Copy link
Contributor

Choose a reason for hiding this comment

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

yes lines are less expensive to render. I think a generic axes plugin would be useful, e.g. for visualizing frames. That could be done by changing the name of this plugin to just AxesPlugin and adding a pose widget.

src/gui/plugins/origin_axes/OriginAxesConfig.qml Outdated Show resolved Hide resolved
src/gui/plugins/origin_axes/OriginAxesConfig.qml Outdated Show resolved Hide resolved
src/gui/plugins/origin_axes/OriginAxesConfig.qml Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde requested a review from iche033 as a code owner May 22, 2020 10:44
@ahcorde
Copy link
Contributor Author

ahcorde commented May 22, 2020

Thank you for the review @chapulina.

I forgot to mention that this PR builds on top of this other PR gazebosim/gz-rendering#87.

I created this other PR gazebosim/gz-rendering#88 to scale the BaseAxisVisual properly.

I can also modify the current implementation of BaseAxisVisual to allow to choose between: arrows or lines

@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 3, 2020

  • Select the origin of the different entities
  • Add an option to set the pose
  • Add an option to change the arrows into lines

axes

LoadAxesbyName(name_entity);
}

void AxesConfig::EntitiesInScene()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chapulina, Is there other way to get the entitie names from a plugin?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you change the plugin to inherit from gazebo::GuiSystem, then you could use an Each call to get for example the names of all visuals, links, collisions, models, etc. The downside is that this plugin would become Gazebo-specific so we can't port it to ign-gui later.

src/gui/plugins/CMakeLists.txt Outdated Show resolved Hide resolved
LoadAxesbyName(name_entity);
}

void AxesConfig::EntitiesInScene()
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change the plugin to inherit from gazebo::GuiSystem, then you could use an Each call to get for example the names of all visuals, links, collisions, models, etc. The downside is that this plugin would become Gazebo-specific so we can't port it to ign-gui later.

src/gui/plugins/axes/AxesConfig.cc Outdated Show resolved Hide resolved
src/gui/plugins/axes/AxesConfig.cc Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
@ahcorde ahcorde requested a review from chapulina August 14, 2020 14:05
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'm still a bit confused about the use case for this plugin. Is it meant to show the world origin, or the origins of entities?

For entities, I think it would be good to also allow attaching to links, collisions, visuals, lights, etc. Basically anything that has a pose component. If we want to keep it not gazebo-specific, then maybe attach to any visual in the scene?

pitch.value = AxesConfig.axesPitch
yaw.value = AxesConfig.axesYaw
}
onAccepted: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Each time I choose a new model, a new visual is created. Is that intentional? I end up with many of them

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chapulina, yes, this is intentional. The GUI allows you to check or uncheck the Show Axes option that enables the axes for the selected entity. I did it like this because you may want to visualize more than one axes on the scene.

Do you expect a different behaviour ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. The usage is still a bit not intuitive to me. For example, if multiple models have axes showing, and I hide the axes for one of them. When I change to another entity, that entity's axes disappear:

axes

I'd expect that when I select the "sphere" from the dropdown, the sphere keeps its axes showing, and the "Show Axes" box checks itself. That is, the widget updates itself to match the currently model on the dropdown.

src/gui/plugins/axes/AxesConfig.qml Outdated Show resolved Hide resolved
src/gui/plugins/axes/AxesConfig.qml Outdated Show resolved Hide resolved
src/gui/plugins/axes/AxesConfig.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 2, 2020

This plugin will create an axes for each entity in the world. I can extend the plugin to accept other types of objects in the scene.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2020
@chapulina chapulina removed the beta Targeting beta release of upcoming collection label Sep 23, 2020
@chapulina
Copy link
Contributor

Removing the beta label, this can go in a minor release.

@chapulina chapulina changed the base branch from master to ign-gazebo4 October 3, 2020 01:30
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.

Since this PR doesn't need anything from ign-gazebo, what do you think of moving it to ign-gui, so other applications can also use it? Now that ign-gazebo can use ignition::gui::events::Render, this plugin doesn't need to be here.

Layout.fillWidth: true
}

ComboBox {
Copy link
Contributor

Choose a reason for hiding this comment

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

All the information on the plugin applies to the entity that's selected on the combo box. So I think it makes sense to put the dropdown on the top of the plugin, above the checkboxes.

@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 17, 2021

Closing this PR in favour of this other PR in ign-gui gazebosim/gz-gui#182

@ahcorde ahcorde closed this Feb 17, 2021
@nkoenig nkoenig deleted the ahcorde/feature/OriginAxesConfig branch March 19, 2021 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants