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

add more light options to service #874

Merged
merged 1 commit into from
Jan 3, 2020

Conversation

christian-rauch
Copy link
Contributor

This PR adds cast_shadows, specular, direction and pose to the SetLightProperties service.

@christian-rauch christian-rauch changed the base branch from kinetic-devel to melodic-devel January 19, 2019 19:10
@christian-rauch christian-rauch changed the base branch from melodic-devel to kinetic-devel January 19, 2019 19:11
@christian-rauch christian-rauch changed the base branch from kinetic-devel to melodic-devel January 19, 2019 19:14
@christian-rauch
Copy link
Contributor Author

This PR is just meant for melodic. Why does the CI test if this can be merged into kinetic? It will obviously fail because the melodic and kinetic branches diverged.

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.

This looks like a great improvement to me. The only problem I see is that we can't tell whether the fields are set or not on the ROS message. So if we release this into Melodic, some values would be zeroed for existing users. i.e. specular would be set to black, cast shadows turned off, the light teleported to the world origin...

My recommendation would be to release this into Noetic instead, and make sure the new fields are documented on the commit message so users see them on the changelog.

A possible solution for Melodic would be to treat zero / false as unset, but that would limit the range of values, and doesn't solve the boolean case.

@christian-rauch
Copy link
Contributor Author

Does the protobuf message store if a value has been set? Because otherwise, these values should be default initialised to 0 anyway. We could reproduce this behaviour with NaNs in place of "unset" values.

But I agree that this should be postponed to the next ROS released then. Shall I rebase this PR to the noetic-devel branch then?

@chapulina
Copy link
Contributor

Does the protobuf message store if a value has been set?

Yes, gazebo::msgs still uses protobuf 2, so there's support for optional fields. You can check if a field is set with light.has_cast_shadows() for example. From protobuf 3 this can only be checked for nested messages though.

Shall I rebase this PR to the noetic-devel branch then?

That would be great, thanks!

@christian-rauch christian-rauch changed the base branch from melodic-devel to noetic-devel January 3, 2020 17:11
@chapulina
Copy link
Contributor

Thank you for rebasing. We need a way to document this so users can find out why their lights that used to stay in place on Melodic are teleported to the origin on Noetic when they change the light's color. This package doesn't have a migration guide though. Do you mind adding some notes about this to the commit message so it shows up on the changelog? Thanks!

The optional 'Light' properties 'cast_shadows', 'specular', 'direction',
and 'pose' are not optional any more. These properties are now set via the
corresponding fields in the ROS message. By default, this will be 0.

ros-simulation#874
@christian-rauch
Copy link
Contributor Author

Do you mind adding some notes about this to the commit message so it shows up on the changelog? Thanks!

Done. I mentioned that these newly added ROS message fields will be initialised to 0 by default.

@chapulina chapulina merged commit a73884f into ros-simulation:noetic-devel Jan 3, 2020
@christian-rauch christian-rauch deleted the light_pose branch January 3, 2020 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants