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

updated loading //material colors #519

Merged
merged 6 commits into from
Mar 23, 2021
Merged

updated loading //material colors #519

merged 6 commits into from
Mar 23, 2021

Conversation

jennuine
Copy link
Collaborator

@jennuine jennuine commented Mar 18, 2021

🦟 Bug fix

Fixes #193

Summary

This fix parses //material color components (i.e., ambient, diffuse, specular, emissive) from a SDFormat file in Material::Load by checking that values are either r,g,b or r,g,b,a (expecting 3 or 4 values) and that each value is between [0,1]. If the checks do not pass, the default color 0,0,0,1 is used.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Code check passed (In source directory, run sh tools/code_check.sh)
  • 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 azeey March 18, 2021 02:28
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 18, 2021
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 18, 2021
@adlarkin adlarkin self-requested a review March 18, 2021 20:50
src/Material.cc Outdated Show resolved Hide resolved
src/Material.cc Outdated Show resolved Hide resolved
src/Material_TEST.cc Outdated Show resolved Hide resolved
src/Material.cc Outdated Show resolved Hide resolved
src/Material.cc Outdated Show resolved Hide resolved
src/Material.cc Outdated Show resolved Hide resolved
src/Material.cc Outdated Show resolved Hide resolved
src/Material_TEST.cc Outdated Show resolved Hide resolved
src/Material_TEST.cc Outdated Show resolved Hide resolved
src/Material_TEST.cc Outdated Show resolved Hide resolved
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@codecov-io
Copy link

codecov-io commented Mar 19, 2021

Codecov Report

Merging #519 (f4633ca) into master (2031028) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
+ Coverage   87.97%   88.01%   +0.04%     
==========================================
  Files          66       66              
  Lines        9546     9579      +33     
==========================================
+ Hits         8398     8431      +33     
  Misses       1148     1148              
Impacted Files Coverage Δ
src/Material.cc 98.80% <100.00%> (+0.29%) ⬆️

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 2031028...f4633ca. Read the comment docs.

Copy link
Collaborator

@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.

Looks pretty good. Can you add a test where a file is being read through the most frequently used user API sdf::Root::Load?


sdf::Material material;
sdf::Errors errors = material.Load(elem);
PrintErrors(errors);
Copy link
Collaborator

Choose a reason for hiding this comment

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

tip: You should be able to do std::cout << errors << std::endl;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine
Copy link
Collaborator Author

Can you add a test where a file is being read through the most frequently used user API sdf::Root::Load?

When working on the integration test I realized that Root::Load does not use Material::Load (i.e., parsing & checking color components were not done) so I moved the color parsing to Param & added the test here: 39c05a5

Copy link
Collaborator

@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.

One comment about exceptions, otherwise looks great!

src/Param.cc Outdated
c = std::stof(token);
colors.push_back(c);
}
catch(const std::exception &/*e*/)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the caught exception be more specific like the ones in ValueFromString

https://github.com/osrf/sdformat/blob/deb142130c6fb96b24a2e0f35e481e34ac014834/src/Param.cc#L509-L520

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup! dbf7614

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using Element::get<Color> has surprising defaults
5 participants