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

Add ResourceRetriever::getFilePath() #972

Merged
merged 8 commits into from
Feb 12, 2018

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Feb 5, 2018

Revision of #970 retargeting to release-6.4.


Motivation
In some cases, it is required to extract the (absolute) file path of a URI (if available). For example, MeshShape needs to know the path to a mesh file to infer the path to a texture image, which is given as a relative path. Currently, we don't have a public method to get a file path from a URI unless it's a file URI. PackageResourceRetriever and DartResourceRetriever have similar functionality, but it's not public.

Proposal
I propose adding ResourceRetriever::getFilePath() that returns the absolute file path specified by a URI or an empty string if unavailable.


Before creating a pull request

  • Document new methods and classes

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@jslee02 jslee02 requested review from mkoval and mxgrey February 5, 2018 05:49
TEST(PackageResourceRetriever, getFilePath_StripsTrailingSlash)
{
#ifdef _WIN32
const char* expected = "file:///" "dart://sample/test/foo";
Copy link
Member

Choose a reason for hiding this comment

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

I'm very unclear about the purpose behind this #ifdef, especially since I believe this will set the value of expected to a const char* which dereferences to file:///dart://sample/test/foo, which doesn't seem like a valid URI (although I could certainly be wrong).

Some commentary about the motivation behind the #ifdef (Why is Windows different? Shouldn't that kind of difference be handled in the Uri class?) would be helpful. As well as the meaning behind strange URI which seems to contain both the file scheme and the dart scheme in sequence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Those invalid URI tests were unintentionally introduced in 162b7a8#diff-2c76e43b1bdbf70403566db01e2e1198. They are fixed by c79de2d.

Some commentary about the motivation behind the #ifdef (Why is Windows different? Shouldn't that kind of difference be handled in the Uri class?) would be helpful.

Added. I hope the comment answers your concern.

@jslee02 jslee02 mentioned this pull request Feb 5, 2018
4 tasks
@codecov
Copy link

codecov bot commented Feb 5, 2018

Codecov Report

❗ No coverage uploaded for pull request base (release-6.4@3059105). Click here to learn what that means.
The diff coverage is 91.11%.

@@              Coverage Diff               @@
##             release-6.4     #972   +/-   ##
==============================================
  Coverage               ?   56.69%           
==============================================
  Files                  ?      310           
  Lines                  ?    23962           
  Branches               ?        0           
==============================================
  Hits                   ?    13586           
  Misses                 ?    10376           
  Partials               ?        0
Impacted Files Coverage Δ
dart/common/LocalResourceRetriever.hpp 100% <ø> (ø)
dart/utils/PackageResourceRetriever.hpp 100% <ø> (ø)
dart/common/ResourceRetriever.hpp 100% <ø> (ø)
dart/utils/DartResourceRetriever.hpp 100% <ø> (ø)
dart/utils/CompositeResourceRetriever.hpp 100% <ø> (ø)
dart/common/ResourceRetriever.cpp 44.44% <0%> (ø)
dart/utils/PackageResourceRetriever.cpp 89.23% <100%> (ø)
dart/utils/CompositeResourceRetriever.cpp 85.71% <100%> (ø)
dart/common/LocalResourceRetriever.cpp 90% <90%> (ø)
dart/utils/DartResourceRetriever.cpp 88.33% <93.33%> (ø)

@jslee02 jslee02 added this to the DART 6.4.0 milestone Feb 5, 2018
@mkoval
Copy link
Collaborator

mkoval commented Feb 6, 2018

It should be possible to get exactly the same behavior using the existing Uri API:

Uri uriToMesh = /* snip */;
std::string relativePathToTexture = /* snip */;
auto uriToTexture = Uri::fromRelativeUri(uriToMesh, relativePathToTexture);

The big advantage of fromRelativeUri() is that it uniformly handles absolute and relative URIs as defined in section 5 of RFC 3986. This means that you can mix schema types (e.g. put a http:// URI in a local file) and all sorts of other goodies.

Implementing getFilePath() only really makes sense for file:// URIs. That largely defeats the purpose of defining the ResourceRetriever interface, which is intended to agnostic to the type of URI being used.

@jslee02
Copy link
Member Author

jslee02 commented Feb 6, 2018

It should be possible to get exactly the same behavior using the existing Uri API:

This may work for a file URI, but how could we get the file path from a non-file URI (e.g., package://... or dart://...) without using PackageResourceRetriever or DartResourceRetriever for instance?

Implementing getFilePath() only really makes sense for file:// URIs.

I see your point and have the same concern, but couldn't figure out a better solution here. 😞

@jslee02
Copy link
Member Author

jslee02 commented Feb 6, 2018

Another solution I came up with was storing the file path to LocalResource. It takes a file path anyway so it should be the easiest solution. The reason I didn't take the solution is that:

  1. I wanted to get the file path without actually opening the file.
  2. It has to downcast to LocalResource to get the file path from.

@mkoval
Copy link
Collaborator

mkoval commented Feb 6, 2018

This may work for a file URI, but how could we get the file path from a non-file URI (e.g., package://... or dart://...) without using PackageResourceRetriever or DartResourceRetriever for instance?

You don't need to get a file path at all - you just need a path to refer to the resource, such that you can retrieve it later. In fact, a file path may not exist at all - the underlying resource retriever could, e.g. use curl to pull the data from the internet on demand.

Suppose you are loading a URDF from the URI dart://my_directory/fruit.urdf using myResourceRetriever as the ResourceRetriever. You start by retrieving() the URI, loading the file's contents, and parsing the XML:

auto urdfUri = Uri::fromString{"dart://my_directory/fruit.urdf"};
auto urdfResource = myResourceRetriever->retrieve(urdfUri);
auto urdfString = read_all(urdfResource.read);
auto urdfModel = parse(urdfString);

Suppose you encounter the relative path meshes/banana.stl in urdfModel and you now want load the mesh into memory. You construct the relative URL from the URI to the URDF file and the relative URI fragment loaded from the file:

auto relativeMeshUri = Uri::fromString{"meshes/banana.stl"}; // loaded from urdfModel
auto meshUri = Uri::fromRelativeUri(urdfUri, relativeMeshUri);
// meshUri now contains "dart://my_directory/meshes/banana.stl"

Now, you can simply load meshUri using myResourceRetriever:

auto meshResource = myResourceRetriever->retrieve(meshUri);

Since you are using the same ResourceRetriever as you used to retrieve the original URDF, it presumably will be able to handle the dart:// URI consistently.


Maybe I am missing something. Why do we need to convert the Uri to a local path before we retrieve it? 🤔

@jslee02
Copy link
Member Author

jslee02 commented Feb 6, 2018

@mkoval All you pointed out make sense to me. I should have said the motivation behind this change. 😅

This was for loading texture images using OSG that requires the file path to the image files (see this). But I couldn't come up with a way to incorporate our resource/retriever mechanism into this (like AssimpInputResourceAdaptor).

@jslee02 jslee02 added pr: ready to merge help wanted Indicates wanting help on an issue or pull request and removed status: ready to merge labels Feb 11, 2018
@jslee02
Copy link
Member Author

jslee02 commented Feb 12, 2018

@mkoval Merging this since this PR is blocking #973. I've created an issue (#984) to continue the above discussion.

@jslee02 jslee02 merged commit a4208af into release-6.4 Feb 12, 2018
@jslee02 jslee02 deleted the 6.4/resource_retriever_getfilepath_v2 branch February 12, 2018 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Indicates wanting help on an issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants