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

Fix tinting of mesh markers #1424

Merged
merged 3 commits into from
Sep 20, 2019

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Sep 9, 2019

Fix #1120. This issue has a long story:

Fortunately, @wjwwood in #1132 added a test program to validate rendering results. However, this test didn't include color-based meshes, which behave differently than texture-based ones.

Originally, i.e. before #752, tinting was achieved by overriding the ambient and diffuse colors of the mesh materials. That worked for texture-based meshes, but not for color-based ones (which just changed their color to the set one, ignoring their original mesh color).
The idea of #752 was to add a new rendering pass to re-render the mesh with the marker color.
In principle, this worked, but it became impossible to make the mesh transparent: The mesh was always rendered unmodified.

This PR additionally adapts the alpha value of the mesh rendering according to the alpha value of the marker color. This seems to yield the expected results:

current rviz #752 / #1388 this PR
mesh-original mesh-pr mesh-pass

Notice, that texture-based meshes are rendered slightly differently now: Setting, like in the example, a red marker color (rbga=[1,0,0,1]) doesn't result in a fully red PR2 base anymore. Instead the originally white base is tinted into red, resulting in a pinkish color. This is the very same effect we yield from tinting a white color-based mesh.
Actually, we would need another alpha parameter to exactly control the transparency / opaqueness of the tinting color separately from the mesh. However, we only have a single alpha parameter available and I decided to use it to control the transparency of the mesh itself, while the tinting transparency is std::min(alpha, 0.5).

Tint the mesh by re-rendering on top of the original one with the marker color.
Also consider the alpha value of the marker color for the mesh.
@rhaschke
Copy link
Contributor Author

Note: The Travis failure is an ABI incompatibility due to a removed, but previously unexposed symbol.
That's not related to this PR.

rhaschke added a commit to rhaschke/rviz that referenced this pull request Sep 10, 2019
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Thanks for continuing the saga :)

The code changes lgtm, but the approach has always been a bit out of my wheel house (meshes and additional material passes). It does make sense how you described it, I just can say if it is "definitely the right approach".

src/test/mesh_marker_test.cpp Show resolved Hide resolved
@wjwwood
Copy link
Member

wjwwood commented Sep 11, 2019

@iche033 can you take a look.

@iche033
Copy link

iche033 commented Sep 12, 2019

The idea of using multi-pass with alpha scene blending makes sense and the code changes also looks good to me. I ran the test and the results produced are expected.

{
// modify material's original color to use given alpha value
Ogre::ColourValue color = pass0->getDiffuse();
color.a = a;
Copy link

Choose a reason for hiding this comment

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

we faced a problem in gazebo before in which the mesh's material is also semi-transparent. We ended up just combining the alpha values, e.g. color.a *= a, so the alpha effect is multiplied. Overriding the value is also fine as long as the user knows what to expect

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, thanks for reviewing.
Multiplying the values is dangerous as multiple updates of the marker's alpha value will continuously decrease the effective alpha value. (We don't have a memory of the original alpha value of the mesh's material.)
To allow to increase the alpha value again, we need to overwrite as proposed.
Hence, the first time the user provides a custom alpha value via the marker's colors, this will take precedence over the mesh's alpha values.

Copy link

Choose a reason for hiding this comment

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

We don't have a memory of the original alpha value of the mesh's material

I see, I was wondering how subsequent updates are applied. Gazebo stores a copy of the original material and uses that as the base when applying further updates. Anyways, this looks good to me.

rhaschke added a commit to rhaschke/rviz that referenced this pull request Sep 20, 2019
@rhaschke rhaschke merged commit 400652d into ros-visualization:melodic-devel Sep 20, 2019
@rhaschke rhaschke deleted the fix-tinting-pass branch September 20, 2019 13:39
@Eberty
Copy link

Eberty commented Oct 25, 2019

@rhaschke Is it possible to do the same thing in kinect devel? I saw that this bug fix was made for melodic-devel only. If you can do that, I will be grateful.

@rhaschke
Copy link
Contributor Author

I do not have resources for this anytime soon. I suggest building melodic-devel from source.

clalancette pushed a commit to ros2/rviz that referenced this pull request Jan 2, 2023
)

Issue: Colors from embedded color based mesh are overridden
and not rendered as expected

It was a known issue on ROS1 that was fixed by
ros-visualization/rviz#1424

Kudos to the original author

Fix #927

Signed-off-by: Xavier BROQUERE <xav.broquere@gmail.com>
mergify bot pushed a commit to ros2/rviz that referenced this pull request Mar 27, 2023
)

Issue: Colors from embedded color based mesh are overridden
and not rendered as expected

It was a known issue on ROS1 that was fixed by
ros-visualization/rviz#1424

Kudos to the original author

Fix #927

Signed-off-by: Xavier BROQUERE <xav.broquere@gmail.com>
(cherry picked from commit 2cb54fc)
clalancette pushed a commit to ros2/rviz that referenced this pull request May 12, 2023
) (#964)

Issue: Colors from embedded color based mesh are overridden
and not rendered as expected

It was a known issue on ROS1 that was fixed by
ros-visualization/rviz#1424

Kudos to the original author

Fix #927

Signed-off-by: Xavier BROQUERE <xav.broquere@gmail.com>
(cherry picked from commit 2cb54fc)

Co-authored-by: Xavier BROQUERE <xav.broquere@gmail.com>
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.

Visualization markers ignore color and show everything as white
4 participants