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 to light if the light is on or off #851

Merged
merged 2 commits into from
Feb 17, 2022
Merged

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented Feb 16, 2022

Signed-off-by: ahcorde [email protected]

🎉 New feature

Summary

Related with:

This will allow to define if a light is on or off.

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@nkoenig
Copy link
Contributor

nkoenig commented Feb 16, 2022

I'm having second thoughts on this. An intensity value of zero is effectively off. Is it possible to use the existing intensity field instead of creating a new field?

@ahcorde
Copy link
Collaborator Author

ahcorde commented Feb 16, 2022

I'm having second thoughts on this. An intensity value of zero is effectively off. Is it possible to use the existing intensity field instead of creating a new field?

This value allows you to turn on and off for a specific value of intensity, otherwise you need to set to zero the intensity and then restore or add a new value to the intensity.

I don't know if this is a good reason to add a new value.

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2022

Codecov Report

Merging #851 (baa8cf5) into sdf11 (61412e6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head baa8cf5 differs from pull request most recent head 706494c. Consider uploading reports for the commit 706494c to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##            sdf11     #851   +/-   ##
=======================================
  Coverage   88.90%   88.91%           
=======================================
  Files          73       73           
  Lines       11079    11086    +7     
=======================================
+ Hits         9850     9857    +7     
  Misses       1229     1229           
Impacted Files Coverage Δ
src/Light.cc 91.76% <100.00%> (+0.35%) ⬆️

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 61412e6...706494c. Read the comment docs.

@nkoenig
Copy link
Contributor

nkoenig commented Feb 16, 2022

I'm having second thoughts on this. An intensity value of zero is effectively off. Is it possible to use the existing intensity field instead of creating a new field?

This value allows you to turn on and off for a specific value of intensity, otherwise you need to set to zero the intensity and then restore or add a new value to the intensity.

I don't know if this is a good reason to add a new value.

That's a good point. I'm okay with this.

@ahcorde ahcorde merged commit 2e396c7 into sdf11 Feb 17, 2022
@ahcorde ahcorde deleted the ahcorde/sdf11/lighton branch February 17, 2022 08:30
@azeey azeey mentioned this pull request Feb 22, 2022
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants