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

Parse file:// mesh prefix from mesh filepath #206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

IDavGal
Copy link

@IDavGal IDavGal commented Jun 13, 2023

Address changes mentioned in this issue.

  • Add support for parsing out the file:// required by rViz from mesh filepaths
  • Normalizes absolute paths after parsing out the prefixes, since when present, the filepath would be considered as relative (filepath starts without /)

@omichel
Copy link
Member

omichel commented Jun 13, 2023

The file URI scheme doesn't allow for relative paths. See explanations here. Relative paths should be expressed without the file:// prefix. Also, absolute paths without a host name should be expressed with three /, e.g. file:///path/to/my_file.txt. Do you have any example of URDF file that features some file:// URI references?

@omichel omichel self-requested a review June 13, 2023 14:52
@IDavGal
Copy link
Author

IDavGal commented Jun 16, 2023

I've encountered this issue when working with an URDF with a workaround for gazebo in ROS 2.

Unrelated, but apparently there is an issue when spawning a robot using gazebo_ros in ROS 2, specifically with mesh filenames using the package:// prefix.
There is a discussion regarding this issue here.
Using file://$(find ) extends to an expression similar to file:///path/to/my/file as you mention.

The URDF I mention can be found here

@omichel
Copy link
Member

omichel commented Jun 19, 2023

I see, therefore, I believe we should support the file:/// protocol which is generated by xacro from the file://$(find syntax.

Comment on lines 598 to +606
if meshfile.count('package'):
idx0 = meshfile.find('package://')
meshfile = meshfile[idx0 + len('package://'):]
elif meshfile.count('file'):
idx0 = meshfile.find('file://')
meshfile = meshfile[idx0 + len('file://'):]
if not os.path.isabs(meshfile):
# Use the path relative to the output file
meshfile = os.path.normpath(os.path.relpath(os.path.join(path, meshfile), outputDirectory))
Copy link
Member

Choose a reason for hiding this comment

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

That would seem clearer and cleaner to me:

Suggested change
if meshfile.count('package'):
idx0 = meshfile.find('package://')
meshfile = meshfile[idx0 + len('package://'):]
elif meshfile.count('file'):
idx0 = meshfile.find('file://')
meshfile = meshfile[idx0 + len('file://'):]
if not os.path.isabs(meshfile):
# Use the path relative to the output file
meshfile = os.path.normpath(os.path.relpath(os.path.join(path, meshfile), outputDirectory))
if meshfile.startswith('package://'):
idx0 = meshfile.find('package://')
meshfile = meshfile[idx0 + len('package://'):] # package relative path
elif meshfile.startswith('file:///'):
idx0 = meshfile.find('file:///')
meshfile = meshfile[idx0 + len('file://'):] # we keep the last slash for absolute path
elif not os.path.isabs(meshfile):
# Use the path relative to the output file
meshfile = os.path.normpath(os.path.relpath(os.path.join(path, meshfile), outputDirectory))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants