Adding flexibility to the lighting environment

[mogumbo]

These changes make it so lights other than RViz's default headlight can be used, such as the ones you find in a plugin like https://github.com/mogumbo/rviz_lighting. The default headlight can now be switched on or off using a global property.

I also changed the default material properties so that specular light from light sources can be seen and so that the full range of ambient light can be seen instead of only half of it. As far as I can tell, there is currently no ambient or specular light coming from light sources in RViz (users would need to be getting that from plugins), so this change should be low-impact.

[wjwwood]

Thanks, we'll have a look when we have time. This looks useful, but anything that touches the material type or lighting is notoriously time consuming to acceptance test in rviz. So please be patient as it might be a while before we have time to review it.

[mogumbo]

Thanks for looking at it. I think the change to ambient material properties is sensible. However, we could go a little further and control specular material with more global properties. The properties could default to a specular value of {0,0,0}, which would be zero impact to current users. Or maybe specular can be optionally specified wherever the diffuse color gets specified.

[lucasw]

It would be nice if the headlight had a color control.

Though it starts to get redundant with rviz_lighting: instead of a special case global option for a head light it would be nice if there was an rviz_lighting light attached to the rviz camera that is on by default (requiring rviz_lighting get merged in here), which suggests that the rviz camera generate a tf frame to hang off of (which might be useful elsewhere), but it would be annoying to have the tf rviz plugin to draw the tf visuals pointing right into the camera so it could have a special case not to draw it.

The additional benefit is that the headlight could be a spotlight or a point light.

[mogumbo]

Agreed. Using the same light code everywhere would be straightforward.

Before absorbing something like rviz_lighting, it might be worth considering if any features should be added first, such as shadows or the ability to specify lights in a URDF (if a light is going to be attached to a robot's frame, it makes a lot of sense to specify it with the rest of the robot).

[mogumbo]

Just curious if there has been any progress on this pull request? I need to get started working with rviz and multiple lights again. I'll just keep using a fork of rviz for now if necessary.

[dhood]

nothing at this stage @mogumbo; we appreciate your patience in the meantime

[dhood]

OK @mogumbo, I've taken a look. I'll comment on the different changes separately.

On the ability to toggle the default light:

I think it's fine to add a toggle to the global options to support users adding custom Light displays. I don't think we should add the ability to modify the default light source because for most users this is not needed, and in cases where the default light source isn't appropriate then custom lights can be used instead. Whether those lights are fixed or attached to the rviz view is a separate question (though if users are looking to represent lighting of an environment more realistically, then perhaps just fixed lights would be sufficient).

I have some comments about the wording used but otherwise the changes related to this look good. Since we aren't adding a new default display we won't have to update default.rviz or anything; users will automatically see this option when they update (in contrast to adding a separate Display for the default light).

---

Regarding the changes to default material properties:

1. Not scaling the colour when calling setAmbient:

   The scaling down is done on purpose so that ambient light doesn't outshine specular/diffuse lighting; the Ogre docs for setAmbient say the ambient colour should be reduced "if you want to see diffuse or specular light effects". So the setAmbient calls should stay as they were.

2. Setting the default specular/shininess property of links/markers.

   Something that complicates this is that while URDF-defined materials can't have light properties set, meshes can (e.g. how shininess is set in this collada mesh). The default specular for meshes is not necessarily determined by rviz, e.g. for collada meshes it's determined by assimp when it imports the mesh (0.4 from what I can tell experimentally, while 0 is used for STL meshes). That's one issue with the current state of this PR: it's over-writing any specular values that had been explicitly set in .dae meshes (I suspect this was unintentional).

   If we set a default for URDF-defined materials, I agree that it won't affect many users today (since specular lights aren't common), but it could impact users in the future. For example, if URDF ever adds support for setting specular properties for materials and a different default is used, then behaviour in rviz would change (models would go from using the rviz default to the urdf parser's default). This isn't necessarily a blocker, but is something that we have to keep in mind.

   While I don't think rviz should mandate the specular/shininess value applied to all URDF-defined materials, we're essentially already doing that by always using the Ogre default of 0. However, which values to use for the defaults is quite subjective, and I am reluctant to have three different defaults for stl meshes, dae meshes and URDF-defined materials. You suggested a global specular property, but I don't think that makes sense in this context because it really should be specified for each material individually. In the absence of that ability in URDF it's not clear that arbitrary defaults are a good idea. I am more leaning towards "if users are concerned about the light properties of materials then they should use formats (e.g. collada meshes) that allow them to specify them".. What do you think, given the additional context?

[dhood]

these comments are only for changes from d6dab41

[dhood]

I would suggest to name this "Default Light" (headlight makes less sense when considering a large-scale visualisation, and suggests it's a spotlight). Unless @wjwwood you have another suggestion for what to call the global directional light?

[dhood]

The use of camera here can be confused with the Camera display type; I'd suggest "Light source attached to the current 3D view."

[dhood]

To make it clearer that this is a toggle, it would be appropriate to name the property default_light_enabled_property_ and rename the function to updateDefaultLightVisible, similar to:

rviz/src/rviz/default_plugin/robot_model_display.cpp

Line 133 in 21a76fa

robot_->setCollisionVisible( collision_enabled_property_->getValue().toBool() );

[mogumbo]

@dhood, thank you for looking at this. I'll make the changes you requested, and then hopefully it can be merged. I'm glad you can accept a toggle for the default light since that's the most important change I made.

1. On ambient light: I disagree with Ogre's instructions on reducing ambient material color: "Reduce this if you want to see diffuse or specular light effects." This would only be true if your ambient light intensity was set unreasonably high. Also, artificially reducing ambient material color a) reduces the range of ambient light that can be observed, b) it's less physically correct for nearly all real-life materials, and c) it adds coupling to the lighting environment, so the programmer needs to consider the material's behavior when setting an ambient light color. That said, I'll change the ambient lines back to what you had before if you still want them that way.

(Out of curiosity, why was the ambient material value set at all if there was no ambient light in RViz? Was there ambient light in the past that got removed at some point?)

2. On specular light: Yes, given the additional context, I wouldn't want to override existing specular material values. It would still be good to have default specular for only URDF and/or a global specular property for when none is defined in material files, but only if it was optional (perhaps coupled to with an "Override material specular" toggle.

[dhood]

I'll make the changes you requested, and then hopefully it can be merged.

When you do so, please push additional commits with the changes/removing previous changes (it's easier for reviewing than force-pushing amended commits) and then we will squash-and-merge at the end.

---

RE ambient light, your points might be valid in a conceptual sense, but we have to work within the technical range of Ogre: FWIU ambient light values represent the minimum lighting of a material, and specular/diffuse effects are added to that value with the final result being clamped to (0,1) range. This is why we want to reduce the ambient component, to reduce the chance of clipping when all light effects are summed. If we don't scale it down, then a white sphere with white ambient light cannot show the diffuse effects:
white sphere with white ambient light and no scaling in setAmbient:

white sphere with white ambient light and current scaling of 0.5 in setAmbient:

I've also recently noticed that this "washed out" effect has been referenced in #841 (comment) when meshes didn't have their ambient colour scaled. So I maintain that the scaling should be left as it was.

Out of curiosity, why was the ambient material value set at all if there was no ambient light in RViz? Was there ambient light in the past that got removed at some point?

Not that I am aware of, but it seems common practice to scale the ambient light back when adding diffuse properties of a material (because of what I mentioned above) so my guess is that it was "just in case". I can't answer with certainty because I wasn't involved in the project at the time.

---

It would still be good to have default specular for only URDF ... for when none is defined in material files

Since it's not possible to define it for URDF-specified materials, rather than be a "default" this would instead essentially be a global property of all URDF-defined materials, and I am of the opinion that a global property does not make sense for this since material properties should be specified for each material individually.

Having come across #841, another reason to not set the light properties to an rviz-specified value is that other applications like gazebo won't use the same light properties. (Additionally, as I mentioned earlier, this value would be different to the defaults used for .stl meshes and that for .dae meshes, even within rviz).

So, while I can appreciate that it's frustrating to not be able to change the light properties of materials specified in URDF, I don't think we can accept the changes to the specular/shininess properties.

For your testing of rviz_lighting you will be able to use collada meshes that allow you to specify light properties of materials. If you really want to use URDF-defined materials with custom light properties, you can consider proposing support for that in the URDF specification, and then rviz could be updated to reflect the values.

[mogumbo]

Changes pushed. Thanks for your help.

The screenshots in #841 imply there was ambient lighting in rviz in 2015. I wonder what happened to it.

So the minimum lighting you'll see on an object is the material_ambient * light_ambient. You can keep colors from saturating by doing something like material_ambient = 0.2 and material_diffuse = 0.8. But this only works if you have one light in your scene. Once you have two or more lights, their contributions can still add to more than one. So the way you usually prevent saturation is by trying to keep light contributions under control. And in practice you would never set light_ambient = 1.0.

But if you want to keep lighting consistent with gazebo like in #841 then that's a whole other ball of string :P

[dhood]

You're right that this current approach won't completely prevent saturation, especially if users can add their own lights.

The screenshots in #841 imply there was ambient lighting in rviz in 2015. I wonder what happened to it.

Yeah I was thinking the same thing... an investigation for another day 😄

Thanks for iterating @mogumbo

[rhaschke]

@dhood Unfortunately this PR broke ABI compatibility by introducing a new variable to VisualizationManager. You should try to avoid that in released versions.
There are lots of custom rviz plugins out there, which crash after this update for no obvious reason if not recompiled.

[dhood]

@rhaschke thanks for pointing this out; that was an oversight on my part. I'll look at adding an automated abi checker to avoid it happening again.