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

Visualization collada texture in marker fix #1084

Conversation

mcopejans
Copy link

Fixes COLLADA mesh material colors as described in issue #751. From the issue thread, it looks like this case has already been fixed by @wjwwood (752), but unfortunately it got unfixed by another PR (893).

@wjwwood
Copy link
Member

wjwwood commented Mar 8, 2017

Thanks for the pr, especially since I haven't had the time to look into the reports of this:

e5ecd45#comments

I feel a bit like a leaf in the wind on this issue, since I don't have a good way of sorting out which solutions to merge or not, which probably led to the "unfix" of this issue. What I'm missing from this is confirmation that we're not "unfixing" something else with this pull request. I'd really like some feedback from @Logane or @brindza or even @hersh.

Otherwise, I'll have to sit down and try to run the "demos" we have to see that we're not breaking some other expected behavior. That will probably take some time for me to get to myself. If anyone else has time to look at the demos with this pr and corroborate with @mcopejans that we're making purely forward progress I'd appreciate and I could merge this directly. Specifically I'm talking about the demos in these images:

#893 (comment)

@mcopejans I don't mean to discount the testing you've done either. Perhaps some before and after images of the tests you did would help too:

Tested with example by @garaemon: https://github.com/garaemon/rviz_collada_marker

@mcopejans
Copy link
Author

mcopejans commented Mar 8, 2017

@wjwwood Thanks for your time, here are some before-after images. Let me know if there is anything I can do to make sure that this PR does not break something else.
Before
image
After
image

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.

I've been doing some testing with this pull request, and it's a definite improvement based on the https://github.com/garaemon/rviz_collada_marker demo. I couldn't find any cases where there is an obvious regression. So I'm going to merge this in the hope it's two steps forward and none backwards.

Thanks @mcopejans for the pr and sorry it took me so long to get to it.

I'm going to forward port this to jade, kinetic and lunar too.

@wjwwood wjwwood merged commit be65d01 into ros-visualization:indigo-devel Apr 29, 2017
@mcopejans mcopejans deleted the fix/marker_texture_visualization_collada branch May 4, 2017 13:39
wjwwood added a commit that referenced this pull request Aug 3, 2017
@wjwwood wjwwood mentioned this pull request Aug 3, 2017
wjwwood added a commit that referenced this pull request Aug 4, 2017
* improve mesh marker test tool

* fix first time color on mesh markers

fixes regression of #1084

* fill out matrix of mesh markers

even though the previously missing ones would be
invisible in the correct behavior

* remove travis in favor of build.ros.org pr testing
wjwwood added a commit that referenced this pull request Aug 21, 2017
* improve mesh marker test tool

* fix first time color on mesh markers

fixes regression of #1084

* fill out matrix of mesh markers

even though the previously missing ones would be
invisible in the correct behavior

* remove travis in favor of build.ros.org pr testing
130s pushed a commit to 130s/rviz that referenced this pull request Aug 21, 2024
…ualization#1085)

* Handle missing effort limit in URDF

Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit a83d8195a16cdcdbd417938cb8d3a30b4c826b12)

Co-authored-by: Patrick Roncagliolo <ronca.pat@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.

2 participants