-
Notifications
You must be signed in to change notification settings - Fork 212
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
Triangle lists support textures #719
Conversation
Signed-off-by: Greg Balke <greg@openrobotics.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you run uncrustify ?
colcon test --event-handlers console_direct+ --ctest-args -R uncrustify
if
should look like (curly brace in the same line as the if ) and 100 characters per line:
if () {
rviz_default_plugins/src/rviz_default_plugins/displays/marker/markers/triangle_list_marker.cpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/marker/markers/triangle_list_marker.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Greg Balke <greg@openrobotics.org>
My bad. I just ran |
Errors are caused by not building against the updated message type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you foresee textures also applying to other marker types in the future? If so, we should take that into consideration when deciding where to put some of the common logic.
rviz_default_plugins/src/rviz_default_plugins/displays/marker/markers/triangle_list_marker.cpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/marker/markers/triangle_list_marker.cpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/marker/markers/triangle_list_marker.cpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/marker/markers/triangle_list_marker.cpp
Outdated
Show resolved
Hide resolved
{ | ||
bool image_loaded = false; | ||
resource_retriever::MemoryResource res = getResource(texture_path); | ||
Ogre::String tex_ext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I prefer full words for variables names to make it clear what they refer to. E.g. memory_resource
and texture_extension
.
rviz_default_plugins/src/rviz_default_plugins/displays/marker/markers/triangle_list_marker.cpp
Outdated
Show resolved
Hide resolved
return !new_message->texture_map.empty() && | ||
new_message->uv_coordinates.size() == new_message->points.size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't appear that this method needs any information about the TriangleListMarker
object. I think it would be better to make this a free function. Then we could alternatively use it for other Marker types as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For things like the arrow marker messages, the points will not equal the uv_coordinates
. This is because the arrow has a pre-determined geometry. In other words, points == coordinates
is particular to this message type.
rviz_default_plugins/src/rviz_default_plugins/displays/marker/markers/triangle_list_marker.cpp
Outdated
Show resolved
Hide resolved
@jacobperron Yes I was! In fact I was hoping for suggestions on where to put the texture loading logic as that should be general to the entire marker system. I copy pasted the |
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
…markers/triangle_list_marker.cpp Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Greg Balke <greg@openrobotics.org>
…triangle_list_textures
…bedded:// Signed-off-by: Greg Balke <greg@openrobotics.org>
return ""; | ||
} | ||
|
||
return new_message->texture_resource.substr(index + 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment here to explain the + 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
@osrf-jenkins retest this please |
Tests should pass following: ros/rosdistro#30563 |
@osrf-jenkins retest this please |
Signed-off-by: Greg Balke <greg@openrobotics.org>
@osrf-jenkins retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Greg Balke <greg@openrobotics.org>
This is tied to ros2/common_interfaces#153. It adds the ability to apply textures defined via URI to arbitrary triangle lists using UV Coordinates.
This functionality is demonstrated here:
https://github.com/gbalke/rviz_shader_tester
There were a couple of things I wasn't sure where to put so I'm holding off on them pending suggestions/review:
getResource
is copied fromrviz_common/load_resource.cpp
. I'm not sure if it's best to expose this function as part of the.hpp
or to instead makeloadImage
part ofload_resource.hpp
.loadImage
may be better namedloadTexture