Add more light options to SetLightProperties (kinetic)

[christian-rauch]

This is a backport of #874 to the kinetic-devel branch.

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

[christian-rauch]

@j-rivero Can you have a look at this?

[christian-rauch]

@j-rivero Can you comment on this PR? Any objections to get this merged?

[chapulina]

I really like this PR in principle, my only concern is backwards-compatibility.

Admittedly, I don't know much about compatibility of ROS messages. Do you know if a service client using the old message would be able to make a successful request to this service once it expects the new message?

[chapulina]

Could you document the new fields?

[christian-rauch]

@chapulina This feature is actually already part of the current noetic branch (#874). This specific PR is just a backport to kinetic and I would therefore refrain from changing anything that is not required to make it work on kinetic to make sure that both branches use the same features.

Admittedly, I don't know much about compatibility of ROS messages. Do you know if a service client using the old message would be able to make a successful request to this service once it expects the new message?

It is source compatible. That is, you can recompile your C++ code or run a python node without the need to change the code. But it is not binary compatible. That means you cannot exchange messages with nodes that use different message definitions.
Maybe this was the reason you agreed to have it in noetic but not in kinetic? If you do not want to break the binary compatibility, I will just close this PR and leave the feature on the noetic branch.

[chapulina]

This feature is actually already part of the current noetic branch

Ah thanks, I knew I had seen this before 😄

Maybe this was the reason you agreed to have it in noetic but not in kinetic? If you do not want to break the binary compatibility, I will just close this PR and leave the feature on the noetic branch.

Yes, that's my main concern. Changing message definitions within a distribution can break existing systems. I think it would be better to leave this in Noetic-only, sorry 😕

[christian-rauch]

I think it would be better to leave this in Noetic-only, sorry

Ok. I will then go ahead and close this to keep the breaking change just in kinetic->noetic.

However, it would now make sense to make noetic-devel the default branch so that the feature is actually distributed to users that check out the repo manually.

[chapulina]

it would now make sense to make noetic-devel the default branch so that the feature is actually distributed to users that check out the repo manually.

That's an interesting idea. Maybe @j-rivero can comment on this. My impression is that this repository's default branch has always been the oldest supported version, so if we change this now it may break their expectations.

[christian-rauch]

My impression is that this repository's default branch has always been the oldest supported version

Well, at least I only check out the git repo if I need the latest version with a feature that hasn't made it to a bloom released version yet. Otherwise, it is much more convenient to just install the binary packages via apt.