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 light_map to material spec #647

Closed
wants to merge 1 commit into from
Closed

Conversation

jennuine
Copy link
Collaborator

🦟 Bug fix

Related to #639, changes will need to be ported to later versions to fully resolve.

Summary

Added missing //light_map from //material/pbr/metal and //material/pbr/specular specification.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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 azeey July 29, 2021 19:13
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jul 29, 2021
@jennuine jennuine mentioned this pull request Jul 29, 2021
@iche033
Copy link
Contributor

iche033 commented Jul 29, 2021

note the matching DOM classes are only available in sdf10, added in #429. It may be ok - it just means that the field just won't be accessible through the PbrWorkflow class in sdf9.

@chapulina
Copy link
Contributor

it just means that the field just won't be accessible through the PbrWorkflow class in sdf9.

I think this can become confusing for downstream users. We should be moving towards parity between the XML spec and the C++ classes.

@jennuine
Copy link
Collaborator Author

note the matching DOM classes are only available in sdf10, added in #429.

I looked at the wrong branch earlier, thank you for pointing that out.

I think this can become confusing for downstream users. We should be moving towards parity between the XML spec and the C++ classes.

I think it's currently confusing for downstream users now since //light_map is in the 1.7 spec but only in sdf10 and not sdf9. I think we should backport #429 to sdf9 for consistency.

I'll close this PR and open a new one with the backport.

@jennuine jennuine closed this Jul 29, 2021
@jennuine jennuine deleted the jennuine/material_spec branch July 29, 2021 23:41
@jennuine
Copy link
Collaborator Author

Backport PR: #648

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