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

Respect spotlight direction #717

Closed
wants to merge 1 commit into from
Closed

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Mar 28, 2021

🦟 Bug fix

Summary

According to SDF and ign-rendering, spot lights have a direction parameter. However, this parameter was ignored when setting the light in the scene. This PR fixes it.

Of course, this might break some scenes which were set up with the direction ignored. However, I still feel that the change is worth it (but I'd suggest the SubT team to thoroughly check if some worlds aren't affected :) ). If merging in Dome is not possible, merge to Edifice, please.

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: Martin Pecka <[email protected]>
@peci1 peci1 requested a review from iche033 as a code owner March 28, 2021 19:29
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Mar 28, 2021
@peci1
Copy link
Contributor Author

peci1 commented Mar 28, 2021

Here's a picture of the current behavior. The link the light is attached to is on the robot's left side with X axis pointing towards the box. I tried many values of the <direction> tag, but it always looks the same. If you think it through and connect with the fact that the default value for <direction> is 0 0 -1, this is exactly it. The light is always shining downwards.

Rotating the link itself rotates the light correctly.

2021-03-28T21:30:52 869741306

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.

If merging in Dome is not possible, merge to Edifice, please.

Yeah fixing this bug in Dome could break current users, targeting Edifice feels safer. Would you be able to retarget the PR to main? We can get it in before the stable release as a critical bug fix. We should also add a note to the migration guide. Thanks!

@peci1
Copy link
Contributor Author

peci1 commented Mar 29, 2021

Okay, replaced by #718. I'm not sure if the light visuals do not need some adjustment too, but that can be fixed anytime later without breaking anything.

@peci1 peci1 closed this Mar 29, 2021
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.

3 participants