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 MeshResourceMarker for mesh with color-based embedded material #928

Merged
merged 1 commit into from
Jan 2, 2023

Conversation

xbroquer
Copy link
Contributor

Issue: Color from embedded color based mesh are overrided and not rendered as expected

It was an 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

@xbroquer
Copy link
Contributor Author

In order to test this PR , I used this repo https://github.com/xbroquer/rviz_collada_marker/tree/ros2
ros2 launch rviz_collada_marker mesh_marker_test.launch.py

Humble PR tested on Humble
Screenshot from 2022-11-25 09-07-49 Screenshot from 2022-11-25 09-04-02

@clalancette clalancette changed the base branch from ros2 to rolling November 28, 2022 14:21
@xbroquer
Copy link
Contributor Author

xbroquer commented Dec 7, 2022

Hello @ahcorde,

is it possible to review this PR ? it will be great if this could be in the coming humble sync :)

Thanks!

@clalancette clalancette self-assigned this Dec 22, 2022
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Style fixes needed. Once those are done, we can run CI on it.

Comment on lines 251 to 252
if (tinting)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, CI is complaining about the style here. This should be:

Suggested change
if (tinting)
{
if (tinting) {

Comment on lines 260 to 262
}
else
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
else
{
} else {

@xbroquer xbroquer force-pushed the ros2-#927 branch 2 times, most recently from a093623 to dc4a3ef Compare December 30, 2022 19:50
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 ros2#927

Signed-off-by: Xavier BROQUERE <xav.broquere@gmail.com>
@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit 2cb54fc into ros2:rolling Jan 2, 2023
@xbroquer
Copy link
Contributor Author

Hi @clalancette ,
Is it possible to backport this change on humble ? Do I have to do something ?

Thanks

@clalancette
Copy link
Contributor

Is it possible to backport this change on humble ?

It looks like it preserves ABI and API, so yes, we can backport it. I'll ask the bot to do it.

@clalancette
Copy link
Contributor

@Mergifyio backport humble

@mergify
Copy link

mergify bot commented Mar 27, 2023

backport humble

✅ Backports have been created

mergify bot pushed a commit 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 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.

color-based meshes rendering issue
2 participants