Resolve URIs relative to file path #1373

iche033 · 2024-02-22T23:23:29Z

🎉 New feature

~~Marked as draft to get feedback. I'll add test and update other resolveURI instances if we decide this is the way to go.~~

This PR is ready for review

Summary

Alternative approach to gazebosim/gz-physics#599 for resolving mesh URI relative to FilePath().

The following URIs are commonly used:

          <mesh>
            <uri>some_local_path/box.obj</uri>
          </mesh>

~~so I extended sdf::findFile to support searching files relative to the model.sdf file path~~.
* Update: Extended resolveURI to take additional search paths when resolving URIs.

To actually load the mesh, we'll also need to call SetStoreResolvedURIs in gz-sim. Note, I also fixed the typo in the SetStoreResolvedURIs function name. We'll need to deprecate the original function in main.

Alternatives considered

The other way is to specify a findFile user callback but it is difficult to do as the user callback needs to take in a different file path (path to model.sdf) for each URI. Given that this relative path URI usage is quite common, I added support directly to sdf::findFile instead.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Ian Chen <[email protected]>

codecov · 2024-02-22T23:49:18Z

Codecov Report

Attention: Patch coverage is 95.34884% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 87.59%. Comparing base (edd5d0b) to head (83af965).
Report is 1 commits behind head on sdf13.

Files	Patch %	Lines
src/ParserConfig.cc	33.33%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            sdf13    #1373   +/-   ##
=======================================
  Coverage   87.58%   87.59%           
=======================================
  Files         128      128           
  Lines       17095    17132   +37     
=======================================
+ Hits        14972    15006   +34     
- Misses       2123     2126    +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scpeters

the concept looks good to me, and I left a few comments

scpeters · 2024-02-23T17:04:45Z

include/sdf/ParserConfig.hh

+
+  /// \brief Get the path to the SDF file.
+  /// \return Full path to SDF file.
+  public: const std::string &FilePath() const;


I would be more specific with this naming, like URIRelativePath because FilePath is already used in the Element class and many other DOM classes that are loaded directly from a file, but since ParserConfig represents configuration, not data loaded from a file, I would use a different name

I gave a shot at naming: SetRelativeURISearchPath. Not tied to it, happy to change to a better name. a83998c

scpeters · 2024-02-23T17:11:54Z

src/SDF.cc

@@ -115,6 +116,16 @@ std::string findFile(const std::string &_filename, bool _searchLocalPath,
    {
      return path;
    }
+    else if (!_config.FilePath().empty())
+    {
+      auto p = std::filesystem::path(_config.FilePath()).parent_path();


I would recommend storing the parent path directly in ParserConfig so that this kind of manipulation isn't required at this level. This could be added to the Set* method in ParserConfig like:

void ParserConfig::SetURIRelativePath(const std::string &, bool _stripNonFolderSuffix)

(the names probably need work)

moved path manipulation logic out of this function in a83998c.

I didn't add the second arg though (_stripNonFolderSuffix) as the new naming should hopefully suggest that it expects a dir path and not a file.

Signed-off-by: Ian Chen <[email protected]>

scpeters · 2024-02-26T21:00:12Z

src/Heightmap.cc

@@ -324,11 +325,14 @@ Errors Heightmap::Load(ElementPtr _sdf, const ParserConfig &_config)
    return errors;
  }

+  auto config = _config;
+  auto path = std::filesystem::path(this->dataPtr->filePath).parent_path();
+  config.SetRelativeURISearchPath(path.string());


I see that SetRelativeURISearchPath is called here to unconditionally override the previous value in the ParserConfig object passed to Load. I think that probably makes sense, I'm trying to think it through to be sure.

Do we want to emit a warning if the value isn't empty before overwriting it? I haven't had a chance to think this through yet.

cc @azeey

I like @azeey's suggestion in #1373 (comment)

azeey · 2024-02-27T23:10:53Z

src/Heightmap.cc

@@ -324,11 +325,14 @@ Errors Heightmap::Load(ElementPtr _sdf, const ParserConfig &_config)
    return errors;
  }

+  auto config = _config;
+  auto path = std::filesystem::path(this->dataPtr->filePath).parent_path();


filePath may not be set if, for example, the object was constructed programmatically or the SDF input was a string. Can you guard against that?

it should just return an empty string. I added a check nevertheless 443dfd2

azeey · 2024-02-27T23:20:33Z

include/sdf/ParserConfig.hh


  /// \brief Get the storeResolvedURIs flag value.
  /// \return True if the parser will attempt to resolve any URIs found and
  /// store them.  False to preserve original URIs
  public: bool StoreResolvedURIs() const;

+  /// \brief Set the path to search for URIs with relative path.
+  /// \param[in] _path Path to search.
+  public: void SetRelativeURISearchPath(const std::string &_path);


I'm concerned that this will be confusing for users, especially since there's a chance for the value to be overwritten. It seems to me we're adding a public API to implement something that should be hidden from the user.

I propose an alternative that does not require adding anything new to ParserConfig:
Let's extend resolveURI to take an optional parent directory as an argument. Then it could do the following:

If the URI is relative:

First do the normal sdf::findFile call

If that fails, prepend the parent directory and try again. (Or we can swap these steps)

If the URI is absolute, just do the normal sdf::findFile.

thanks for suggestions. I implemented the proposed changes in 70e897e. The resolveURI function now takes an optional set of search paths for resolving URIs.

Signed-off-by: Ian Chen <[email protected]>

azeey · 2024-03-01T20:09:13Z

test/integration/resolve_uris.cc

+  EXPECT_TRUE(sdf::filesystem::exists(scriptUri));
+  EXPECT_TRUE(sdf::filesystem::exists(meshVisualUri));
+  EXPECT_TRUE(sdf::filesystem::exists(meshColUri));
+}


I would like to see a test where the input is an SDF string, not a file, so as to exercise the situation where sdf::Element::FilePath returns an empty string. It's not clear to me what parent_path returns in that case.

added test for loading sdf from string. In this case, parent_path will be empty string so it's then up to the user callback function to search for the uri file. c130219

Signed-off-by: Ian Chen <[email protected]>

azeey · 2024-03-04T15:58:12Z

@iche033 Looks like there's a build error on windows.

Signed-off-by: Ian Chen <[email protected]>

iche033 · 2024-03-04T19:08:07Z

@iche033 Looks like there's a build error on windows.

fixed in f51bd4b

scpeters

just some minor suggestions; looks good to me

src/Utils.cc

test/integration/resolve_uris.cc

Signed-off-by: Ian Chen <[email protected]>

iche033 added 2 commits February 22, 2024 23:07

resolve URI wrt to file path

eb74c67

Signed-off-by: Ian Chen <[email protected]>

check empty filepath

0314f80

Signed-off-by: Ian Chen <[email protected]>

iche033 requested review from azeey and scpeters as code owners February 22, 2024 23:23

iche033 marked this pull request as draft February 22, 2024 23:23

github-actions bot added the 🎵 harmonic Gazebo Harmonic label Feb 22, 2024

iche033 changed the base branch from sdf14 to sdf13 February 22, 2024 23:23

iche033 added 🌱 garden Ignition Garden and removed 🎵 harmonic Gazebo Harmonic labels Feb 22, 2024

iche033 mentioned this pull request Feb 22, 2024

bullet-featherstone: Fix loading mesh from relative path gazebosim/gz-physics#599

Closed

8 tasks

scpeters reviewed Feb 23, 2024

View reviewed changes

update function name to SetRelativeURISearchPath, add tests

a83998c

Signed-off-by: Ian Chen <[email protected]>

iche033 marked this pull request as ready for review February 23, 2024 19:50

This was referenced Feb 23, 2024

Enable StoreResolvedURIs when loading SDF gazebosim/gz-sim#2323

Closed

bullet-featherstone: Improve mesh collision stability gazebosim/gz-physics#600

Merged

scpeters reviewed Feb 26, 2024

View reviewed changes

azeey reviewed Feb 27, 2024

View reviewed changes

iche033 added 4 commits February 28, 2024 21:49

add search paths arg to resolveURI

70e897e

Signed-off-by: Ian Chen <[email protected]>

remove unused var

f092180

Signed-off-by: Ian Chen <[email protected]>

check abs path

e973d80

Signed-off-by: Ian Chen <[email protected]>

line wrap

143f1b0

Signed-off-by: Ian Chen <[email protected]>

azeey requested changes Mar 1, 2024

View reviewed changes

add test for sdf string

c130219

Signed-off-by: Ian Chen <[email protected]>

iche033 requested a review from azeey March 2, 2024 00:12

check empty filepath

443dfd2

Signed-off-by: Ian Chen <[email protected]>

fixing windows filesystem path

f51bd4b

Signed-off-by: Ian Chen <[email protected]>

azeey approved these changes Mar 4, 2024

View reviewed changes

scpeters approved these changes Mar 4, 2024

View reviewed changes

src/Utils.cc Show resolved Hide resolved

src/Utils.cc Show resolved Hide resolved

test/integration/resolve_uris.cc Outdated Show resolved Hide resolved

test/integration/resolve_uris.cc Outdated Show resolved Hide resolved

Resovlve -> Resolve, add comments

83af965

Signed-off-by: Ian Chen <[email protected]>

scpeters approved these changes Mar 5, 2024

View reviewed changes

iche033 merged commit 2246db8 into sdf13 Mar 5, 2024
13 checks passed

iche033 deleted the resolve_uri_filepath branch March 5, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve URIs relative to file path #1373

Resolve URIs relative to file path #1373

iche033 commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading

scpeters left a comment

scpeters Feb 23, 2024

iche033 Feb 23, 2024 •

edited

Loading

scpeters Feb 26, 2024

scpeters Feb 23, 2024

iche033 Feb 23, 2024

scpeters Feb 26, 2024

scpeters Feb 26, 2024

scpeters Feb 28, 2024

azeey Feb 27, 2024

iche033 Mar 2, 2024

azeey Feb 27, 2024

iche033 Feb 28, 2024

azeey Mar 1, 2024

iche033 Mar 2, 2024

azeey commented Mar 4, 2024

iche033 commented Mar 4, 2024

scpeters left a comment

Resolve URIs relative to file path #1373

Resolve URIs relative to file path #1373

Conversation

iche033 commented Feb 22, 2024 • edited Loading

🎉 New feature

Summary

Alternatives considered

Checklist

codecov bot commented Feb 22, 2024 • edited Loading

Codecov Report

scpeters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iche033 Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azeey commented Mar 4, 2024

iche033 commented Mar 4, 2024

scpeters left a comment

Choose a reason for hiding this comment

iche033 commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading

iche033 Feb 23, 2024 •

edited

Loading