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

Support for generic URIs #464

Merged
merged 58 commits into from Aug 5, 2015
Merged

Conversation

@mkoval
Copy link
Collaborator

@mkoval mkoval commented Jul 22, 2015

This pull request adds full-fledged URI support to DART. This generalizes the rudimentary support for package:// URIs in DartLoader to the other file loaders and for loading meshes. I believe that this pull request is fully backwards compatible (with one minor exception), so I'd like to get this into DART 5.1.

The key idea is that external resources should be specified by URIs, not paths, nested resources should resolved relative to their parent URI, and it should be possible for the user to add support for custom URIs without modifying DART.

This pull request adds the following classes:

  1. common::ResourceRetriever provides a standard interface for opening am URI and returning a...
  2. common::Resource provides file-like access to a resource using an fread-like API
  3. common::LocalResourceRetriever provides access to local paths and file:// URIs using the standard library
  4. common::Uri class that provides URI parsing and merging functionality with no dependencies (painstakingly implemented from RFC 3986)
  5. utils::PackageResourceRetriever resolves package:// URIs to file:// URis using a user-specified mapping. This mimics the behavior of addPackageDirectory on DartLoader
  6. utils::SchemaResourceRetriever adds support for using multiple ResourceRetrievers at the same time; e.g. to support both file:// and package:// URIs.
  7. dynamics::AssimpResourceInputResourceAdaptor maps between the DART's Resource and ResourceRetriever classes and the corresponding Assimp IOFile and IOSystem classes. This allows Assimp to transparently load meshes that consist of multiple files (e.g. external textures).

It also makes the following API changes:

  1. MeshShape optionally takes a ResourceRetriever argument. If specified, this is made available through a getResourceRetriever function, e.g. to resolve textures using the same ResourceRetriever as was used to load the mesh.
  2. MeshShape now supports arbitrary URIs, which are accessible using the getMeshUri method. If the URI is a file:// URI, then it is also available (for backwards compatibility) via the getMeshPath method.
  3. SdfParser and SkelParser methods now take an optional ResourceRetriever argument and use it resolve both the input URI and any embedded URIs. If not specified, the argument defaults to a LocalResourceRetriever.
  4. SkelParser now resolves meshes relative to the .skel file instead of assuming that they are in DART_DATA_PATH. This technically breaks backwards compatibility. However, it makes the class so much more useful that I think it's worth it.
  5. DartLoader now optionally accepts a ResourceRetriever argument (see above). If not specified, it defaults to a SchemaResourceRetriever that uses a LocalResourceRetriever to resolve file:// URIs and a PackageResourceRetriever to resolve package:// URIs.
  6. DartLoader's addPackageDirectory function is deprecated in favor of passing a custom ResourceRetriever.
  7. DartLoader is refactored to use primarily static methods. In DART 6.0, we can remove addPackageDirectory and provide an entirely static API.

These features add no dependencies if the compiler has full C++11 support. Unfortunately, the version of libstdc++ shipped with GCC 4.8 (this is the version distributed on Ubuntu 14.04) and below have a broken implementation of std::regex. There isn't a reliable way of testing the version of libstdc++ in both GCC and Clang, so I fall back on Boost.Regex if I detect libstdc++ being used at all.

This is a big pull request, so I am sure I missed something. Please post if you have any questions or see anything that may break backwards compatibility.

, mPackageRetriever(new utils::PackageResourceRetriever(mLocalRetriever))
, mRetriever(new utils::SchemaResourceRetriever)
{
using namespace std::placeholders;

This comment has been minimized.

@jslee02

jslee02 Aug 1, 2015
Member

Do we need this line?

namespace dart {
namespace utils {

DartLoader::DartLoader()
: mLocalRetriever(new common::LocalResourceRetriever)

This comment has been minimized.

@jslee02

jslee02 Aug 1, 2015
Member

  : mLocalRetriever(new common::LocalResourceRetriever()),
    mPackageRetriever(new utils::PackageResourceRetriever(mLocalRetriever)),
    mRetriever(new utils::SchemaResourceRetriever())
const size_t size = resource->getSize();
std::string content;
content.resize(size);
if(resource->read(&content.front(), size, 1) != 1)

This comment has been minimized.

@jslee02

jslee02 Aug 1, 2015
Member

I found similar code in DartLoader::readFileToString().

It would be useful and safer if we provide Resource::read(std::string& content, size_t size, size_t count) that stores that file contents into std::string rather than writing similar code in different places.

This comment has been minimized.

@mkoval

mkoval Aug 4, 2015
Author Collaborator

Adding that method would you let you omit the resize() line here. I don't think this justified adding another method to the Resource interface.

If we find that we need this function somewhere else, then I propose that we add a helper function somewhere in utils.

@jslee02
Copy link
Member

@jslee02 jslee02 commented Aug 1, 2015

Most comments I made in the code are about code style. Strictly speaking, we don't have written code convention and furthermore I've followed different code styles myself so far. I tried to comment for something settled styles but you can skip those style issue for now. We might begin to write up our code convention in not too far future and check code style more strictly from that time.

Also, note that CloudResourceRetriever doesn't need to be part of this pull request.

@jslee02 jslee02 added this to the DART 5.1.0 milestone Aug 1, 2015
@mkoval
Copy link
Collaborator Author

@mkoval mkoval commented Aug 4, 2015

I think I addressed all of your comments. 😓

Please take another look and let me know if I missed anything. Github's not great at displaying diffs of this size, so it's hard to tell if I got them all.


/// ResourceRetriever provides methods for testing for the existance of and
/// accessing the content of a resource specified by URI.
class ResourceRetriever {

This comment has been minimized.

@jslee02

jslee02 Aug 4, 2015
Member

class ResourceRetriever
{
common::ResourcePtr retrieve(const std::string& _uri) override;

private:
std::vector<common::ResourceRetrieverPtr> getRetrievers(const std::string& _uri);

This comment has been minimized.

@jslee02

jslee02 Aug 4, 2015
Member

Exceeded 80 columns. Please wrap this line as:

  std::vector<common::ResourceRetrieverPtr> getRetrievers(
      const std::string& _uri);
std::vector<common::ResourceRetrieverPtr> mDefaultResourceRetrievers;
};

typedef std::shared_ptr<CompositeResourceRetriever> CompositeResourceRetrieverPtr;

This comment has been minimized.

@jslee02

jslee02 Aug 4, 2015
Member

Exceeded 80 columns. Please wrap this line as:

using CompositeResourceRetrieverPtr
    = std::shared_ptr<CompositeResourceRetriever>;
std::string& _packageName, std::string& _relativePath);
};

typedef std::shared_ptr<PackageResourceRetriever> PackageResourceRetrieverPtr;

This comment has been minimized.

@jslee02

jslee02 Aug 4, 2015
Member

using PackageResourceRetrieverPtr = std::shared_ptr<PackageResourceRetriever>;
@jslee02
Copy link
Member

@jslee02 jslee02 commented Aug 4, 2015

Thanks @mkoval ! I found more minor formatting issues. Once these are modified then we could merge this PR if there is no other feedback.

@mxgrey
Copy link
Member

@mxgrey mxgrey commented Aug 5, 2015

Looks good to me 👍

@mkoval
Copy link
Collaborator Author

@mkoval mkoval commented Aug 5, 2015

I think I fixed the remaining formatting issues.

@jslee02
Copy link
Member

@jslee02 jslee02 commented Aug 5, 2015

👍 Nice work!

jslee02 added a commit that referenced this pull request Aug 5, 2015
@jslee02 jslee02 merged commit d71f20c into dartsim:master Aug 5, 2015
2 checks passed
2 checks passed
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants