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

Triangle lists support textures #719

Merged
merged 11 commits into from
Aug 27, 2021
Merged

Conversation

gbalke
Copy link
Contributor

@gbalke gbalke commented Jun 25, 2021

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 from rviz_common/load_resource.cpp. I'm not sure if it's best to expose this function as part of the .hpp or to instead make loadImage part of load_resource.hpp.
  • loadImage may be better named loadTexture

Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke gbalke added the enhancement New feature or request label Jun 25, 2021
Copy link
Contributor

@ahcorde ahcorde left a 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 () {

Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke
Copy link
Contributor Author

gbalke commented Jun 29, 2021

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 () {

My bad. I just ran ament_uncrustify --remormat which should take care of this!

@gbalke
Copy link
Contributor Author

gbalke commented Jun 29, 2021

Errors are caused by not building against the updated message type.

Copy link
Member

@jacobperron jacobperron left a 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.

{
bool image_loaded = false;
resource_retriever::MemoryResource res = getResource(texture_path);
Ogre::String tex_ext;
Copy link
Member

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.

Comment on lines 333 to 334
return !new_message->texture_map.empty() &&
new_message->uv_coordinates.size() == new_message->points.size();
Copy link
Member

@jacobperron jacobperron Jul 15, 2021

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.

Copy link
Contributor Author

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.

@gbalke
Copy link
Contributor Author

gbalke commented Jul 15, 2021

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.

@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 getResource function from https://github.com/ros2/rviz/blob/ros2/rviz_common/src/rviz_common/load_resource.cpp as mentioned in the original comment. I could put the loadImage function in the same place or somewhere more specific to the visualization plugin depending on your recommendation.

Greg Balke and others added 5 commits August 3, 2021 15:28
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>
@gbalke
Copy link
Contributor Author

gbalke commented Aug 20, 2021

CI up to rviz:

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

…bedded://

Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke
Copy link
Contributor Author

gbalke commented Aug 20, 2021

Wrong package name...

CI up to rviz2*:

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

return "";
}

return new_message->texture_resource.substr(index + 3);
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@ahcorde
Copy link
Contributor

ahcorde commented Aug 24, 2021

@osrf-jenkins retest this please

@gbalke
Copy link
Contributor Author

gbalke commented Aug 24, 2021

Tests should pass following: ros/rosdistro#30563

@gbalke
Copy link
Contributor Author

gbalke commented Aug 26, 2021

@osrf-jenkins retest this please

Signed-off-by: Greg Balke <greg@openrobotics.org>
@ahcorde
Copy link
Contributor

ahcorde commented Aug 27, 2021

@osrf-jenkins retest this please

Copy link
Contributor

@ahcorde ahcorde left a 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>
@gbalke
Copy link
Contributor Author

gbalke commented Aug 27, 2021

CI up to rviz2:

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

@gbalke gbalke merged commit b30097c into ros2:ros2 Aug 27, 2021
@gbalke gbalke deleted the triangle_list_textures branch August 27, 2021 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants