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 intensity to light.proto #131

Merged
merged 2 commits into from
Feb 8, 2021
Merged

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Feb 4, 2021

Added light intensity field to light.proto. Related with this issue gazebosim/gz-rendering#232

Signed-off-by: ahcorde [email protected]

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #131 (c2d4277) into main (446fc89) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #131   +/-   ##
=======================================
  Coverage   84.78%   84.78%           
=======================================
  Files           7        7           
  Lines         828      828           
=======================================
  Hits          702      702           
  Misses        126      126           

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 446fc89...8d42568. Read the comment docs.

@@ -55,6 +55,7 @@ message Light
float spot_inner_angle = 13;
float spot_outer_angle = 14;
float spot_falloff = 15;
float intensity = 18;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put it in order below? Also, please document all new fields.

Signed-off-by: ahcorde <[email protected]>
@@ -61,4 +61,7 @@ message Light

/// \brief Unique id of light's parent
uint32 parent_id = 17;

/// \brief light intensity
float intensity = 18;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we default this to 1.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicit default values are not allowed in proto3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iche033 can we merge this then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we can force this as a required field

Copy link
Contributor

Choose a reason for hiding this comment

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

ok I see, lets leave it as is then.

@ahcorde ahcorde requested a review from iche033 February 5, 2021 09:12
@ahcorde ahcorde merged commit 6d3c2d4 into main Feb 8, 2021
@ahcorde ahcorde deleted the ahcorde/feature/intensity branch February 8, 2021 06:48
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.

3 participants