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.

Sign up for GitHub

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

Resolve URIs relative to file path #1373

Merged
merged 11 commits into from
Mar 5, 2024
Merged

Resolve URIs relative to file path #1373

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
eb74c67
resolve URI wrt to file path
iche033 Feb 22, 2024
0314f80
check empty filepath
iche033 Feb 22, 2024
a83998c
update function name to SetRelativeURISearchPath, add tests
iche033 Feb 23, 2024
70e897e
add search paths arg to resolveURI
iche033 Feb 28, 2024
f092180
remove unused var
iche033 Feb 28, 2024
e973d80
check abs path
iche033 Feb 28, 2024
143f1b0
line wrap
iche033 Feb 28, 2024
c130219
add test for sdf string
iche033 Mar 2, 2024
443dfd2
check empty filepath
iche033 Mar 2, 2024
f51bd4b
fixing windows filesystem path
iche033 Mar 4, 2024
83af965
Resovlve -> Resolve, add comments
iche033 Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion include/sdf/ParserConfig.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,10 @@ class SDFORMAT_VISIBLE ParserConfig
/// and merge the child link into the parent.
public: bool URDFPreserveFixedJoint() const;

/// \brief Set the storeResolvedURIs flag value.
/// \sa SetStoreResolvedURIs
public: void SetStoreResovledURIs(bool _resolveURI);

/// \brief Set the storeResolvedURIs flag value.
/// \param[in] _resolveURI True to make the parser attempt to resolve any
/// URIs found and store them. False to preserve original URIs
Expand All @@ -188,13 +192,21 @@ class SDFORMAT_VISIBLE ParserConfig
/// If the FindFileCallback provides a non-empty string, the URI will be
/// stored in the DOM object, and the original (unresolved) URI will be
/// stored in the underlying Element.
public: void SetStoreResovledURIs(bool _resolveURI);
public: void SetStoreResolvedURIs(bool _resolveURI);

/// \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 the SDF file.
/// \param[in] _path Full path to SDF file.
public: void SetFilePath(const std::string &_path);

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@iche033 iche033 Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

looks good


/// \brief Private data pointer.
GZ_UTILS_IMPL_PTR(dataPtr)
};
Expand Down
4 changes: 3 additions & 1 deletion src/Mesh.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,11 @@ Errors Mesh::Load(ElementPtr _sdf, const ParserConfig &_config)

if (_sdf->HasElement("uri"))
{
auto config = _config;
config.SetFilePath(this->dataPtr->filePath);
this->dataPtr->uri = resolveURI(
_sdf->Get<std::string>(errors, "uri", "").first,
_config, errors);
config, errors);
}
else
{
Expand Down
20 changes: 20 additions & 0 deletions src/ParserConfig.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ class sdf::ParserConfig::Implementation

/// \brief Flag to expand URIs where possible store the resolved paths
public: bool storeResolvedURIs = false;

/// \brief Path to file where this sdf is loaded
public: std::string filePath;
};


Expand Down Expand Up @@ -183,6 +186,12 @@ bool ParserConfig::URDFPreserveFixedJoint() const

/////////////////////////////////////////////////
void ParserConfig::SetStoreResovledURIs(bool _resolveURI)
{
this->SetStoreResolvedURIs(_resolveURI);
}

/////////////////////////////////////////////////
void ParserConfig::SetStoreResolvedURIs(bool _resolveURI)
{
this->dataPtr->storeResolvedURIs = _resolveURI;
}
Expand All @@ -193,3 +202,14 @@ bool ParserConfig::StoreResolvedURIs() const
return this->dataPtr->storeResolvedURIs;
}

/////////////////////////////////////////////////
void ParserConfig::SetFilePath(const std::string &_path)
{
this->dataPtr->filePath = _path;
}

/////////////////////////////////////////////////
const std::string &ParserConfig::FilePath() const
{
return this->dataPtr->filePath;
}
11 changes: 11 additions & 0 deletions src/SDF.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

#include <cstdlib>
#include <filesystem>
#include <fstream>
#include <sstream>
#include <functional>
Expand Down Expand Up @@ -115,6 +116,16 @@
{
return path;
}
else if (!_config.FilePath().empty())
{
auto p = std::filesystem::path(_config.FilePath()).parent_path();
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

👍

p = p / filename;
path = p.string();
if (sdf::filesystem::exists(path))
{
return path;

Check warning on line 126 in src/SDF.cc

View check run for this annotation

Codecov / codecov/patch

src/SDF.cc#L126 

Added line #L126 was not covered by tests
}
}
}

// Next check the install path.
Expand Down
Loading