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

Edit material ambient/specular/diffuse/emissive in component inspector #1123

Merged
merged 11 commits into from
Oct 29, 2021

Conversation

jennuine
Copy link
Contributor

🎉 New feature

Summary

Ability to edit a model's visual ambient, specular, diffuse, and emissive material colors through the component inspector using sliders or a color dialog.

Peek 2021-10-19 14-00

Test it

  1. ign gazebo shapes.sdf
  2. In entity tree, expand any model > expand the link > select the visual
  3. In component inspector, expand the material and play with sliders and/or color dialog

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>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Oct 19, 2021
@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #1123 (ba862c2) into ign-gazebo4 (e151ba8) will decrease coverage by 0.68%.
The diff coverage is 5.36%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo4    #1123      +/-   ##
===============================================
- Coverage        67.00%   66.32%   -0.69%     
===============================================
  Files              244      245       +1     
  Lines            18263    18468     +205     
===============================================
+ Hits             12237    12248      +11     
- Misses            6026     6220     +194     
Impacted Files Coverage Δ
.../plugins/component_inspector/ComponentInspector.cc 5.05% <0.00%> (-0.97%) ⬇️
.../plugins/component_inspector/ComponentInspector.hh 28.57% <ø> (ø)
src/gui/plugins/shapes/Shapes.cc 31.25% <ø> (ø)
src/systems/user_commands/UserCommands.cc 52.22% <4.41%> (-6.60%) ⬇️
src/rendering/RenderUtil.cc 46.74% <12.96%> (-1.94%) ⬇️
include/ignition/gazebo/components/VisualCmd.hh 100.00% <100.00%> (ø)

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 e151ba8...ba862c2. Read the comment docs.

@jennuine jennuine marked this pull request as draft October 19, 2021 22:25
@jennuine
Copy link
Contributor Author

Need to address saving.

// Visual service
std::string visualService
{"/world/" + worldName + "/visual_config"};
this->dataPtr->node.Advertise(visualService,
Copy link
Contributor

Choose a reason for hiding this comment

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

check service name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other services in UserCommands::Configure do not check, should this one differ?

src/systems/user_commands/UserCommands.cc Show resolved Hide resolved
@nkoenig
Copy link
Contributor

nkoenig commented Oct 25, 2021

I've update the Figma design to include color selection. Can you update this to match?

Here is a screenshot of the relevant portion:

colors

Clicking on the color swatch opens the color picker.

@chapulina
Copy link
Contributor

Regarding the updated design, one thing to consider is that the GUI would be accepting hex as input, while the SDF spec accepts decimals. I don't think that's a huge deal, but it can become cumbersome for users going back-and-forth between the 2. It also becomes tricky to verify that your SDF was properly loaded.

@nkoenig
Copy link
Contributor

nkoenig commented Oct 25, 2021

Regarding the updated design, one thing to consider is that the GUI would be accepting hex as input, while the SDF spec accepts decimals. I don't think that's a huge deal, but it can become cumbersome for users going back-and-forth between the 2. It also becomes tricky to verify that your SDF was properly loaded.

I took the hex approach in the GUI because it's more compact than listing RGBA, users are probably equally familiar with the hex approach, and I'd like to update the color picker modal to have a color wheel + RGBA + hex all on one panel like in gimp.

Ultimately, I'm trying to get the GUI to be more intuitive and user friendly instead of designing the GUI to the fit the needs of underlying libraries like SDF.

@nkoenig
Copy link
Contributor

nkoenig commented Oct 25, 2021

I experimented with an RGBA option, and I'm happy with it as long as the values are in the range 0-255 in order to avoid the floating point precision problem. We might want to think about support the 0-255 range in SDF as well.

Here is a picture of the updated design:
color

@chapulina
Copy link
Contributor

We might want to think about support the 0-255 range in SDF as well.

I ticketed this a while back: gazebosim/sdformat#195

Now that pose accepts degrees and quaternions, I think there's a precedent to adding flexibility to other tags too.

and I'd like to update the color picker modal to have a color wheel + RGBA + hex all on one panel like in gimp.

Agree. I think that the native color dialog on the latest Ubuntu is very limited compared to what it used to be. On Focal, even for Gazebo classic, I see something close to this:

image

While in the past it used to be like this:

image

I haven't dug to see at which point that changed, and whether we can force Qt to use the old one. Ideally, we wouldn't need to reinvent a color picker ourselves.

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine jennuine marked this pull request as ready for review October 28, 2021 00:07
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine
Copy link
Contributor Author

Need to address saving.

Saving will be addressed outside this PR.


Here is the new design and was able to force Qt to use the old color dialog.

Peek 2021-10-27 17-06

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.

Functionality and design looks good.

@jennuine jennuine requested a review from ahcorde October 28, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants