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

Marker textures #153

Merged
merged 9 commits into from
Aug 20, 2021
Merged

Marker textures #153

merged 9 commits into from
Aug 20, 2021

Conversation

gbalke
Copy link
Contributor

@gbalke gbalke commented Jun 25, 2021

This is a pre-cursor to a corresponding update to the RViz triangle marker message system. It allows for a texture to be applied to triangles using UV coordinates.

The functionality has been demonstrated here:
https://github.com/gbalke/rviz_shader_tester

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

Just so you know, this is quite a large change. In particular, putting this in will basically invalidate all rosbags that were taken with previous ROS distributions, and it ensures that Humble will be wire-incompatible with Galactic (cross-distribution compatibility isn't something we guarantee, but it is nice to keep when we can).

All of that said, we do allow for this kind of breakage in the core if the change has broad enough applicability. Can you describe more about UV coordinates, and why we might want them here?

@gbalke
Copy link
Contributor Author

gbalke commented Jun 28, 2021

Sure thing! UV Coordinates are used to describe the relationship of a point in a geometric feature to a corresponding image in a texture file. I think wikipedia describes this well. As it pertains to robotics, if someone wishes to visualize dynamic geometries with an applied texture, such as a collision mesh with heat map, they can easily do so by specifying these values and having their rendering backend (Rviz -> Ogre) handle applying this texture. UV Coordinates are well established within graphics and this exposes that feature in a direct way to be used with the ros marker message system.

I understand the hope to maintain backwards compatibility. I do know that there are message formats out there, such as protobuf, that have managed to maintain forwards and backwards compatibility and allow for the addition of new features. I am curious if this is something that can be introduced to ros messages so new features can be added without causing damage/major issues.

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Overall I think that this is a good extension of the message. However we need to make sure that the message remains self contained.

I know that often textures are maintained in parallel with meshes and are reused. But keeping things self contained is super valuable in this context. Alternatively we should be setting up a parallel stream possibly. But that's getting complicated again.

visualization_msgs/msg/Marker.msg Outdated Show resolved Hide resolved
visualization_msgs/msg/UVCoordinate.msg Outdated Show resolved Hide resolved
visualization_msgs/msg/Marker.msg Outdated Show resolved Hide resolved
@jacobperron
Copy link
Member

FWIW, it might be worth taking a look at mesh_tools for inspiration. They appear to have message definitions for textures and UV coordinates.

@gbalke
Copy link
Contributor Author

gbalke commented Jul 16, 2021

FWIW, it might be worth taking a look at mesh_tools for inspiration. They appear to have message definitions for textures and UV coordinates.

I'm happy to go this route if the uri is going to be replaced by an alternate message stream. I think UUID implies a specific implementation which we don't necessarily want. A unique name will work. I think texture_id with a comment describing this should do the trick?

@tfoote
Copy link
Contributor

tfoote commented Jul 21, 2021

Great find @jacobperron We should definitely leverage the work that mesh_tools has already demonstrated. As they've shown we can encapsulate the texture relatively easily in a message. Then everything will be self contained and there's no issues with synchronization, access, or id mapping.

Greg Balke added 2 commits August 3, 2021 15:26
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke
Copy link
Contributor Author

gbalke commented Aug 3, 2021

After chatting with @tfoote, we came to the decision to include the texture in the message. This was done to provide an ease-of-access to apply textures. While sub-optimal for certain use-cases, this will be recordable and be quick to bring up. For more optimized solutions, more complicated packages may be used such as mesh_utils.

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'm concerned with the approach selected. It deviates from the already existing MESH_RESOURCE marker type and it is not future proof, in the case that you want to use another method of specifying the texture.

I would suggest these fields instead:

# If texture_uri either empty string ("") or something like "texture_embedded://",
# then the texture can be read from texture_image, otherwise it is loaded via
# resource_retriever, just like mesh_resource.
string texture_resource
sensor_msgs/CompressedImage texture_image
UVCoordinate[] uv_coordinates

With code like:

if (
  marker_msg.texture_resource.empty() ||
  marker_msg.texture_resource == "texture_embedded://")
{
  load_texture_from_compressed_image(marker_msg.texture_image);
} else {
  // load_texture_from_resource_retriever(marker_msg.texture_resource);
  throw std::runtime_error("not implemented");  // or some similar error condition
}

That way if/when you want to support loading via package:// or file://, you don't have to change the message definition again.

visualization_msgs/msg/Marker.msg Outdated Show resolved Hide resolved
Greg Balke added 2 commits August 16, 2021 16:04
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke
Copy link
Contributor Author

gbalke commented Aug 16, 2021

The latest changes match @wjwwood's suggestions. I've also added a mesh field at the request of @tfoote, @clalancette, @wjwwood to give meshes the same ability to be recorded in rosbag if one chooses to do so. Implementation will not immediately be available but this is a preemptive change to minimize number of commits modifying this message.

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

This is looking good from our discussion. Structurally I think it's basically there. I mostly want to improve the documentation to make sure that we and everyone following are on the same page.

@@ -56,8 +56,13 @@ geometry_msgs/Point[] points
# NOTE: alpha is not yet used
std_msgs/ColorRGBA[] colors

# Texture resource is a special URI that can either reference a texture file in
# a format acceptable to libcurl or:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add to the documentation references to resource retriever and use it like we already do for the meshes. (This lets us use package:// syntax too)

https://index.ros.org/p/resource_retriever/

Then for the embedded approach I might suggest embedded://name or attached://name and then we can mirror it and used the same keyword for the mesh.

visualization_msgs/msg/Mesh.msg Outdated Show resolved Hide resolved
visualization_msgs/CMakeLists.txt Outdated Show resolved Hide resolved
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 second @tfoote's comments and also I think generally we could use more detailed docs, because I find myself asking things like:

  • what URI are acceptable
  • What is the "texture_name" used for, supposed to be?

And generally the Mesh docs could be updated to match.

visualization_msgs/CMakeLists.txt Outdated Show resolved Hide resolved
visualization_msgs/msg/Mesh.msg Outdated Show resolved Hide resolved
Signed-off-by: Greg Balke <greg@openrobotics.org>
@tfoote tfoote requested a review from wjwwood August 18, 2021 01:40
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.

One doc fix up needed, otherwise lgtm

visualization_msgs/msg/Marker.msg Outdated Show resolved Hide resolved
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke
Copy link
Contributor Author

gbalke commented Aug 18, 2021

Complete CI via --packages-above-and-dependencies:

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

@gbalke
Copy link
Contributor Author

gbalke commented Aug 19, 2021

CI after merging (IDL changes made last build unstable):

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

@gbalke gbalke merged commit 4b1ba46 into ros2:master Aug 20, 2021
@gbalke gbalke deleted the marker_textures branch August 20, 2021 19:52
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.

6 participants