Enabling proper URI resolution for World URDF parsing

[mxgrey]

The changes that were made to allow for correct URI resolution had an unintended side effect of breaking the relative path resolution for URDF files that represent an entire World (as opposed to the standard use of a URDF file, which is to represent an individual robot).

This pull request fixes this issue and allows World URDF files to correctly resolve relative paths.

[psigen]

Does it make sense to just delete this dead code and the above comment?

[mxgrey]

Yes, I should actually do a second pass through the code and delete a bunch of old things that aren't being used anymore.

[psigen]

👍 for clean code.

[mkoval]

The relative to absolute resolution looks good to me. There's also a fair amount of code cleanup in here. 👍

[jslee02]

It seems it's reversed.

} // namespace urdf_parsing
} // namespace utils
} // namesapce dart

[jslee02]

Could we add a test to show it doesn't work without this fix?

[mxgrey]

I've added a test which fails prior to the changes of this pull request, and I've removed the code that is no longer being used.

[jslee02]

Thanks! Looks good to me. 👍

[ana-GT]

Thank you Grey! :)

[jturner65]

i just pulled down master and i am getting a compilation error : parsePose not found (line 182 urdf_world_parser.cpp). i saw the declaration was deleted as part of the update here - can this reference be deleted as well?

[mxgrey]

Are the headers for urdf different between Windows and Linux? If you put the declaration back in, does that allow it to work? If so, we may as well just keep the declaration; maybe it was originally there to accommodate Windows.

[jturner65]

putting it back in allowed it to compile, but i am now getting an error with the file loading - the "uri.mScheme.get_value_or("file") " check on line 79 of LocalResourceRetriever.cpp is getting back "D" which is causing it to fail to load the file.

i noticed that that files and paths are being built in DART with forward slashes - is this new? windows doesn't like those, relic from the DOS days.

[jturner65]

should i make this an issue?

[mxgrey]

If the URI methods aren't working in Windows, then it's definitely worth creating an issue.

I was under the impression that Windows accepted forward slashes as equivalent to backslashes, but perhaps that's not the case.

One possible solution might be to use preprocessor directives in the Uri class so that it uses forward slash for Unix-based systems and backslash for Windows. There could be a private const static member variable called mSlash or something, and a preprocessor directive could make it a forward slash or backslash depending on the platform.

[mkoval]

Windows does support both types of slashes, with one obscure exception:

The "?" prefix can also be used with paths constructed according to the universal naming convention (UNC). To specify such a path using UNC, use the "?\UNC" prefix. For example, "?\UNC\server\share", where "server" is the name of the computer and "share" is the name of the shared folder. These prefixes are not used as part of the path itself. They indicate that the path should be passed to the system with minimal modification, which means that you cannot use forward slashes to represent path separators, or a period to represent the current directory, or double dots to represent the parent directory. Because you cannot use the "?" prefix with a relative path, relative paths are always limited to a total of MAX_PATH characters.

I suspect that the problem is related to the drive letter, not the forward slashes. For example, the windows path C:/foo/bar is parsed as a URI with schema C and path /foo/bar. You would have to pass this as the URI file:///C:/foo/bar. This is consistent with the way Internet Explorer treats file:// URIs on Windows.

My preference would be to require the file:/// prefix for absolute paths on Windows. If that is too burdensome, we could consider adding a hack that converts C:/foo/bar to file:///C:/foo/bar. Modern versions of windows only allow 26 drives letters, so simply look for schemas named A-Z.

[mxgrey]

This definitely seems like a tricky situation. For the sake of URI correctness, I would be more inclined to require having file:// on any string that gets to be converted to a URI for Windows. However, I'm not a Windows user, so I wouldn't be affected by this burden and therefore I don't feel like I have the authority to impose this burden on Windows users.

One option might be to have a function like

bool Uri::fromLocalPath(const std::string& _input);

which would check for a file: scheme and then if the scheme is read as anything else (such as C:, D:, etc), it will append a file: scheme and then reparse the Uri.

I think this would give us a way to avoid manually prepending file:/// to all the paths of Windows files without needing any questionable hacks.

[psigen]

Quick question: This only affects absolute paths on windows, right? How many of these are there in the codebase vs. relative paths? Could we just change them to resolve?

Really, I think that requiring URI strings for resources (absolute or relative) is a very good candidate for a breaking API change on the next major rev due to the vastly improved versatility and standardization of URI formatting. I would recommend making the minimal change necessary here to deal with non-URI paths with the intent to deprecate it all later.

I might even go so far as to add a printed warning to the workaround, alerting users that that the path should be changed to a file URI.

[jturner65]

A possible solution could be to modify whatever is generating the macro DART_DATA_PATH and possibly DART_ROOT_PATH in config.h (the cmake generated file) to hold the appropriate configuration (adding the " file://" to the macro def for windows builds). this is the only place these absolute paths are referenced, as far as i know (can't find any other - everything else uses these macros).

so if, as i understand it (please correct me if i am wrong), we need to change the format of the absolute paths for windows, maybe we can at least do so here.

to be honest, i'd prefer some kind of function that returned a string of the path prefixes (build and data)that could at the very least then be overridden if necessary, to a macro. i never really liked the idea of putting a path prefix in a macro - it seems too heavy handed for something like this.

would this be possible (i.e. building a function in config.h that would return the data or build path as strings instead of having the macros)?

[mxgrey]

to be honest, i'd prefer some kind of function that returned a string of the path prefixes (build and data)that could at the very least then be overridden if necessary, to a macro.

I do feel that macros tend to be rather gross, but they do have some value to offer, for lack of a better macro system in C++. I think in the particular case of the DART_DATA_PATH, a macro makes more sense than a function for a couple reasons:

1. A macro is completely transparent whereas a function would only be transparent if we have it inline (which I suppose would be fine).
2. The only way we could override it if it were a function would be to make it a class member function and have it require an instance of the class in order to be called. At least with a macro you can use #undef DART_DATA_PATH follow by #define DART_DATA_PATH "/my/custom/path" to override it. Granted, keeping track of macro definitions is a huge pain when the #undef directive is involved, but this still seems better to me than embedding it in a dummy class in order to override it.

A possible solution could be to modify whatever is generating the macro DART_DATA_PATH and possibly DART_ROOT_PATH in config.h (the cmake generated file) to hold the appropriate configuration (adding the " file://" to the macro def for windows builds).

I definitely agree that this should be done 👍 . But I don't think it solves the larger problem of having users who might want to specify an absolute path on Windows using strings.

I don't want casual users to have to understand the nuances of URIs in order to use files in DART. For that matter, I myself don't want to have to think about the nuances of URIs when dealing with files in DART. My proposal would be:

1. Alter every function that takes in a filename as a string and instead have it take in a Uri argument.
2. Have an implicit constructor for the Uri class that takes in a string an converts it to a Uri (equivalent to the existing fromString(~) function). This should allow backwards compatibility.
3. Have static member functions for the Uri class that match up with the existing fromString(~), fromStringOrPath(~), and fromRelativeUri(~) functions, but which return a Uri instance instead of a boolean. They could be named something like createFromString(~), createFromStringOrPath(~), and createFromRelativeUri(~).
4. Also provide a bool Uri::fromPath(std::string) function which enforces a scheme of file://, and a static Uri Uri::createFromPath(std::string) which returns a Uri instance that is created by passing the string into Uri::fromPath(~).

So this way, the user does not need to worry about the details of a Uri and does not need to check whether a string for a filename is correctly using the file:// scheme. They can simply run the string through Uri::createFromPath(~) as if it's a filter, and everything should be okay.

[psigen]

This links up with the issue that JS reported earlier in #488.

[psigen]

@mxgrey: I think that's a pretty reasonable way to go about it. As someone who is often transparently switching to over-the-network and in-memory stream resources and moving between OSes, I'm exactly on the other side of the fence where I really don't want to have to see/touch raw file paths in my code.

Static converter functions to a clear URI type seem like a safe way to encapsulate both of these use cases.

[jslee02]

i just pulled down master and i am getting a compilation error : parsePose not found (line 182 urdf_world_parser.cpp). i saw the declaration was deleted as part of the update here - can this reference be deleted as well?

I realized that the urdf package in the DART prerequisite installer was old one, which has missing parsePose() declaration in the header. The missing declaration was added by urdf 0.2.9. In #510, I updated the DART prerequisite installer that includes urdf 0.3.0 that is the same version on Ubuntu utopic/vivid/wily. So the missing declaration issue should be resolved with the updated installer. Please find it in the wiki page.

[jturner65]

@jslee02 the urdf issue is addressed with this change. Thanks!

On the issue of paths, and as an extension of @mxgrey 's great ideas, could a system be implemented that had dart look at a single location for an xml file that would hold uri strings keyed by namespace/project/resource name/whatever? so instead of passing around filenames or uri's, we'd pass around default or user-defined string constants like "skelFile" in the code, which would then be used, along with whatever qualifiers were deemed necessary, as a key to look up the uri in the xml file. i think this would be a better method of accessing files/resources than hardcoding paths/uri's - only 1 uri would ever be coded into dart, and in the xml tied to that uri all other resource locations could be specified by the user. changes could be made without recompilation, and platform specific configurations could be specified easily.

[mkoval]

We could introduce a custom URI schema for built in models. For example,

dart://cube.skel

could refer to cube.skel in the appropriate directory. Thoughts?

On Fri, Oct 2, 2015 at 2:49 PM John Turner notifications@github.com wrote:

@jslee02 https://github.com/jslee02 the urdf issue is addressed with
this change. Thanks!

On the issue of paths, and as an extension of @mxgrey
https://github.com/mxgrey 's great ideas, could a system be implemented
that had dart look at a single location for an xml file that would hold uri
strings keyed by namespace/project/resource name/whatever? so instead of
passing around filenames or uri's, we'd pass around default or user-defined
string constants like "skelFile" in the code, which would then be used,
along with whatever qualifiers were deemed necessary, as a key to look up
the uri in the xml file. i think this would be a better method of accessing
files/resources than hardcoding paths/uri's - only 1 uri would ever be
coded into dart, and in the xml tied to that uri all other resource
locations could be specified by the user. changes could be made without
recompilation, and platform specific configurations could be specified
easily.

—
Reply to this email directly or view it on GitHub
#497 (comment).

[mxgrey]

I think the idea of a dart URI scheme is really good, and we can use that for @jslee02 's idea of storing models outside of the repo. We would store the models on a server, and when the dart scheme gets used, the resource retriever would check for a local copy of the file; if the file does not exist, it will retrieve it from the server and save a local copy in a special directory.

I think this would also address @jturner65 's complaints about our use of macros for locating files that get shipped with DART.

This raises two big questions in my mind:

1. Where should the "special" local directory be located on the local computer? Should it be in a hidden directory in the user's home directory, such as ~/.dart/models/, or should it be in a system-wide directory to avoid pointless duplication, like /opt/dart/models/?
2. To what extent should we perform version control for the models? The resource retriever can easily know what version of DART it was compiled with, so it shouldn't have any trouble with fetching the correct version. But on the server's file system and local file system, we wouldn't want to duplicate all the models with each release (e.g. have models/v5_0/, models/v5_1/, and models/v5_2/ where all the models in each directory are identical).

One idea that comes to mind for (2) is we could have an index.txt file in each version directory that contains a list of the files that were available as of that version and points to the most recent version of the file that predates the version number of the current directory. In other words, if you compiled with v5.1 and you ask for dart://cube.skel, the DartResourceRetriever would look at models/v5_1/index.txt and look for a cube.skel entry. Then the entry for cube.skel would say that version 5.1 of cube.skel has not had any changes since v4.3, so the DartResourceRetriever would fetch models/v4_3/cube.skel. Then there would be no need to have a copy of models/v5_1/cube.skel whatsoever.

[mkoval]

The versioning system you described sounds very similar to the `model://` URI and manifest file system used by Gazebo. It might be worth looking at their documentation/implementation for inspiration.

[jslee02]

Also provide a bool Uri::fromPath(std::string) function which enforces a scheme of file://, and a static Uri Uri::createFromPath(std::string) which returns a Uri instance that is created by passing the string into Uri::fromPath(~).

I'm currently working on revising Uri class based on the discussion above in portable_uri branch. However, I'm not clear the differences between Uri::createFromPath(std::string) and Uri::createFromStringOrPath(std::string). Uri::createFromPath(std::string) always assume that the input string contains file:// while Uri::createFromStringOrPath(std::string) doesn't?

[mxgrey]

Uri::createFromStringOrPath(std::string) will accept whatever the scheme of the string is. If the string has no scheme at all, then it will use a scheme of file. (So Uri::fromStringOrPath does not need to be changed at all from how it currently works)

Uri::createFromPath(std::string) will insist on having a scheme of file. If the incoming string is interpreted as having scheme of file, then we accept it and return. However, if the incoming string is interpreted as having any other scheme, then we prepend file:/// to the original string input and reinterpret it.

[jslee02]

Uri::createFromPath(std::string) will insist on having a scheme of file. If the incoming string is interpreted as having scheme of file, then we accept it and return. However, if the incoming string is interpreted as having any other scheme, then we prepend file:/// to the original string input and reinterpret it.

Here are four examples:

(1) file:///foo/bar.txt  -->  file:///foo/bar.txt
(2) /foo/bar.txt  -->  file:///for/bar.txt
(3) other_scheme:foo/bar -->  file:///other_scheme:foo/bar.txt
(4) other_scheme:foo/bar -->  file:///foo/bar.txt

(1), (2) are clear. So if we prepend file:/// to the original string then (3) is correct but not (4), right? I think (3) makes sense since : is allowed for file paths.

[mxgrey]

Right, we definitely want (3) and not (4). The original motivation for this was to robustly handle the way Windows paths begin with C:, D:, etc.

[jslee02]

OK, now I get the idea of Uri::createFromPath(std::string).

I revised Uri::getStringOrPath(~) to be able to parse Windows paths correctly. It first determine whether the incoming string is path or not. Here is the criteria:

• The path should be absolute path.
• Windows path begin with single drive letter and followed by :// or :\.
• Unix path begin with a forward slash.

So I wonder we still need Uri::createFromPath(std::string) when Uri::createFromPath(std::string) can parse most possible paths for both of Unix and Windows. If there is a more special path that Uri::getStringOrPath(~) cannot parse it with the above logic, then we might need it.

[mxgrey]

I'm a bit concerned about the changes you're proposing to getStringOrPath. I realize it's unlikely, but what if someone somewhere has a scheme which is only a single-letter? I feel like it would be better to not hardcode workarounds for specific cases like Windows path names.

Moreover, as you said before, colons are allowed to be in path names, but what you've proposed will only catch the particular way that Windows uses colons to specify drives.

[jslee02]

I'm a bit concerned about the changes you're proposing to getStringOrPath. I realize it's unlikely, but what if someone somewhere has a scheme which is only a single-letter? I feel like it would be better to not hardcode workarounds for specific cases like Windows path names.

I basically agree with that. But then how getFromStringOrPath() can parse Windows style path with drive letters? If we want to left getFromStringOrPath() incomplete anyway then I would like to remove it and have two separate functions: getFromString and getFromPath.

Moreover, as you said before, colons are allowed to be in path names, but what you've proposed will only catch the particular way that Windows uses colons to specify drives.

With the absolute path restriction, it can be easily handled by regarding all the strings after C:/. where it contains colons or not.

[mxgrey]

I think I would prefer having getFromString and getFromPath as two separate functions instead of having a special case for Windows files in getFromStringOrPath.

Does anyone have a strong reason for wanting to keep getFromStringOrPath?

[mkoval]

We should definitely have separate getFromString and getFromPath functions. I added the getFromStringOrPath function to support backwards compatibility for files that reference resources by path.

However, this may not actually be necessary. These paths are resolved relative to the URI of the source file, so the schema should be inherited accordingly.

[psigen]

Strong agreement w/ @mkoval here, getFromString and getFromPath have clear semantics, and give users a firm idea of whether a file path or URI string is expected.

[mxgrey]

If we separate them out into two functions, I think getFromPath should still check whether or not the incoming string already has the file:/// pattern on the front of it, and leave it out of the mPath URI component if it's there.

[jslee02]

I'm about to implement getFromPath something like:

bool Uri::fromPath(const std::string& _path)
{
  // TODO(JS): We might want to check validity of _path.

  static const std::string fileScheme("file://");

  if (_path.compare(0, fileScheme.length(), fileScheme) != 0)
  {
#ifdef _WIN32
    return fromString(fileScheme + "/" + _path);
#else
    return fromString(fileScheme + _path);
#endif
  }

  return fromString(_path);
}

Here we assume that _path is absolute path, and add file:/// for Windows and file:// for Unix since Unix-style path already has / at the front.

[jslee02]

PR #517 (for DART 5.1) is created that resolves issues of file URI.

DART_ROOT_PATH and dart:// scheme will be addressed in the next version of 5.1.

[jturner65]

this looks great!