Add resolve_filepath for url scheme such that package://, model:// #2
Conversation
|
So sorry for the delayed reply, just saw this! Nice PR, I'll test it and merge it soon. Best, |
|
btw, I've tested this (at least for |
|
Hi @mmatl I also wanted this functionality so was looking into why this was failing... there's a few issues I found, but it seems the tests won't pass even on master? the |
There was a problem hiding this comment.
Hi @iory nice addition! It works well for viewing a normal URDF, but breaks the relative structure... with the two changes I suggested I can view both, but the tests still fail because of bad dae files. I'm not sure if @mmatl can address that or not. I definitely will keep using this repo and this PR so it would be good to see it merged!
| @@ -199,6 +218,7 @@ def get_filename(base_path, file_path, makedirs=False): | |||
| The resolved filepath -- just the normal ``file_path`` if it was an | |||
| absolute path, otherwise that path joined to ``base_path``. | |||
| """ | |||
| file_path = resolve_filepath(base_path, file_path) | |||
| fn = file_path | |||
| if not os.path.isabs(file_path): | |||
| fn = os.path.join(base_path, file_path) | |||
There was a problem hiding this comment.
when given a relative path, this gives the wrong path. resolve_filepath returns base_path + file_path, then this if makes fn = base_path + base_path + file_path. Should this be os.path.join(os.getcwd(), file_path) instead?
| if os.path.exists(resolved_filepath): | ||
| return resolved_filepath | ||
| dirname = os.path.dirname(dirname) | ||
| return False |
There was a problem hiding this comment.
in the tests, this will return false, then in get_filename, os.path.isabs fails because it's trying to evaluate False
| @@ -199,6 +218,7 @@ def get_filename(base_path, file_path, makedirs=False): | |||
| The resolved filepath -- just the normal ``file_path`` if it was an | |||
| absolute path, otherwise that path joined to ``base_path``. | |||
| """ | |||
| file_path = resolve_filepath(base_path, file_path) | |||
There was a problem hiding this comment.
with comment above, I think you need a check to see if resolve_filepath returns False
| @@ -199,6 +218,7 @@ def get_filename(base_path, file_path, makedirs=False): | |||
| The resolved filepath -- just the normal ``file_path`` if it was an | |||
| absolute path, otherwise that path joined to ``base_path``. | |||
| """ | |||
| file_path = resolve_filepath(base_path, file_path) | |||
There was a problem hiding this comment.
| file_path = resolve_filepath(base_path, file_path) | |
| resolved_file_path = resolve_filepath(base_path, file_path) | |
| if resolved_file_path != False: | |
| file_path = resolved_file_path |
| @@ -199,6 +218,7 @@ def get_filename(base_path, file_path, makedirs=False): | |||
| The resolved filepath -- just the normal ``file_path`` if it was an | |||
| absolute path, otherwise that path joined to ``base_path``. | |||
| """ | |||
| file_path = resolve_filepath(base_path, file_path) | |||
| fn = file_path | |||
| if not os.path.isabs(file_path): | |||
| fn = os.path.join(base_path, file_path) | |||
There was a problem hiding this comment.
| fn = os.path.join(base_path, file_path) | |
| fn = os.path.join(os.getcwd(), file_path) |
This PR supports URL scheme such that
package://,model://.With this PR, we can load urdf contains following XML tag.