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

Allow examples and tutorials to be built out of DART source tree #842

Merged
merged 14 commits into from
Feb 5, 2017

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Jan 31, 2017

This PR allows the examples and tutorials can be built out of the DART source tree when DART is installed on the system. The examples and tutorials are now installed under system directory (e.g., for Linux, /usr/share/doc/dart/examples and /usr/share/doc/dart/tutorials) with sample data files (/usr/share/doc/dart/data) when DART is installed. You can copy the directories to your workspace to build them. Detailed instruction can be found in the directories.

We are still able to build examples and tutorials through the DART build system. It requires distinct build script, which is in the main CMakeLists.txt, because the build scripts in the examples and tutorials assumes that DART is already installed which is not always true when building DART from source. One downside of doing this would be that we should keep maintaining two build scripts for the same projects.

SampleResourceRetriever class is introduced to be able to load the sample data files from either of the system directory or in DART source. It first tries to load from the system directory and then from the system directory if failed. A specific URI is also introduced to refer to the sample data (e.g., file://sample/skel/biped.skel).

TODOs:

  • Add instruction at the root of examples and tutorials
  • Add debian packages for examples and tutorials (will be done by other PR)
  • Add Travis-CI tests for out-of-source build of examples and tutorials if possible (will be done by other PR)

Note: This PR will be squashed and then merged.

Related issues: #751, #752, #839

This change is Reviewable

@jslee02 jslee02 added this to the DART 6.2.0 milestone Jan 31, 2017
@jslee02 jslee02 changed the title [WIP] Allow examples and tutorials to build out-of-source Allow examples and tutorials to build out-of-source Feb 1, 2017
@jslee02
Copy link
Member Author

jslee02 commented Feb 1, 2017

@mxgrey, @mkoval, @psigen Please take a look at this PR if interested. 😄

@mxgrey
Copy link
Member

mxgrey commented Feb 1, 2017

Reviewed 232 of 232 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


dart/utils/sdf/SdfParser.cpp, line 100 at r1 (raw file):

using JointMap = std::map<std::string, SDFJoint>;

simulation::WorldPtr readWorld(tinyxml2::XMLElement* worldElement,

It looks like this should be bumped down a line for formatting.


dart/utils/sdf/SdfParser.cpp, line 174 at r1 (raw file):

    dynamics::ShapeNode* shapeNode);

void readVisualizationShapeNode(dynamics::BodyNode* bodyNode,

Also should be bumped down a line.


dart/utils/sdf/SdfParser.cpp, line 184 at r1 (raw file):

    const common::ResourceRetrieverPtr& retriever);

void readAspects(const dynamics::SkeletonPtr& skeleton,

Also should be bumped down a line


examples/CMakeLists.txt, line 19 at r1 (raw file):

add_subdirectory(softBodies)
add_subdirectory(speedTest)
add_subdirectory(vehicle)

I know CMake considers it better practice to explicitly list all subdirectories/files, but that's just because doing so ensures that the "make" command will rerun CMake any time a new subdirectory/file gets added. I expect that the DART source tree will remain pretty static (except for DART developers), so I don't really see this as necessary (since I think DART developers can be trusted to know when to rerun CMake manually).

I'm not necessarily opposed to explicitly listing them out, but I do prefer automating the build system when possible, because that means less development overhead.


Comments from Reviewable

@mxgrey
Copy link
Member

mxgrey commented Feb 1, 2017

I've published a Reviewable review, but I'll add one more high-level comment:

I very much like these changes. I like the flexibility it offers to users of DART.

I get the impression that the build system for the examples was changed to reflect the best practices according to the developers of CMake rather than the practices that might be most convenient from a development standpoint. If that's the case, I can accept that. I somewhat appreciated the genericity of the CMakeLists.txt files that we were using in the example folders, but admittedly they do fly in the face of CMake's official stance on best practices.

@jslee02
Copy link
Member Author

jslee02 commented Feb 1, 2017

Yes, exactly. I intended that so that the users can build the first DART example without being bothered to understand too many CMake things.


Review status: all files reviewed at latest revision, 4 unresolved discussions.


dart/utils/sdf/SdfParser.cpp, line 100 at r1 (raw file):

Previously, mxgrey (Michael X. Grey) wrote…

It looks like this should be bumped down a line for formatting.

Done.


dart/utils/sdf/SdfParser.cpp, line 174 at r1 (raw file):

Previously, mxgrey (Michael X. Grey) wrote…

Also should be bumped down a line.

Done.


dart/utils/sdf/SdfParser.cpp, line 184 at r1 (raw file):

Previously, mxgrey (Michael X. Grey) wrote…

Also should be bumped down a line

Done.


examples/CMakeLists.txt, line 19 at r1 (raw file):

Previously, mxgrey (Michael X. Grey) wrote…

I know CMake considers it better practice to explicitly list all subdirectories/files, but that's just because doing so ensures that the "make" command will rerun CMake any time a new subdirectory/file gets added. I expect that the DART source tree will remain pretty static (except for DART developers), so I don't really see this as necessary (since I think DART developers can be trusted to know when to rerun CMake manually).

I'm not necessarily opposed to explicitly listing them out, but I do prefer automating the build system when possible, because that means less development overhead.

I totally agree with you. But the reason I did this is to leave the CMake scripts in examples and tutorials as plain as possible. The CMake scripts are for new users to DART and maybe even new ones to building C++ projects using CMake. I would like to lower the entry barrier to get started with DART as much as possible.

On the other hand, the CMake script for building examples and tutorials in DART source tree are much more complex but (hopefully) convenient to the developers (it uses predefined custom macros and other CMake features).

You can see that I still used the listing way even for the in source building. The reason to do so was because of the different target library dependencies. But I'm also open to use file(GLOB ...) to gather the list of subdirectories/files with the superset of dependencies.


Comments from Reviewable

@psigen
Copy link
Collaborator

psigen commented Feb 2, 2017

I think this is a great set of changes. The only question that I have is regarding the use of the SampleResourceRetriever in conjunction with a file://sample/ URI. This strikes me as a bit strange because this URI is not treated as a typical file:// URI at all.

It seems like as-is, this could introduce two issues:

  1. A new user cuts and pastes one of the URIs into another piece of code using a LocalResourceRetriever, which attempts to resolve it and fails. It's unclear to the new user why this file:// URI is not working, since the LocalResourceRetriever will think it can resolve this scheme.
  2. Someone actually has a resolvable sample hostname that makes this URI resolution actually valid as a plain file URI accessing some path on a machine with the hostname sample and LocalResourceRetriever can resolve it to an unintended host.

Would it make sense to use a custom URI scheme such as sample:// or even dart:// or dart-sample:// for these resources to clearly indicate that they are not simple file URIs?

@mkoval
Copy link
Collaborator

mkoval commented Feb 2, 2017

This is great! I think this is a great use for the ResourceRetriever API. It's nice that the examples only need one extra line of code (creating the SampleResourceRetriever) to access data.

I agree with @psigen that I'd prefer to use use a custom schema over file:// for the reasons that he listed above.

@jslee02
Copy link
Member Author

jslee02 commented Feb 2, 2017

I also agree on @psigen's suggestion. The schema was initially sample:// at first. The reason I changed to file:// was that I thought it refers to all kinds of local resources, which I don't think so now. Maybe this confusion was originated from the name of the class that resolves file:// schema is LocalResourceRetriever. So I'm thinking of changing the name to FileResourceRetriever. How do you think?

@jslee02
Copy link
Member Author

jslee02 commented Feb 2, 2017

I've changed:

  • SampleResourceRetriever to DartResourceRetriever
  • URI pattern from file://sample/skel/shapes.skel to dart://sample/skel/shapes.skel

@jslee02 jslee02 changed the title Allow examples and tutorials to build out-of-source Allow examples and tutorials to be built out of DART source tree Feb 2, 2017
@jcnorby
Copy link

jcnorby commented Feb 2, 2017

Good timing - I am a new user to DART, CMake, and C++, so perhaps I can test out the accessibility of the tutorials/getting started in general.

@jslee02
Copy link
Member Author

jslee02 commented Feb 5, 2017

Thank you for the comments! We could consider the renaming of FileResourceRetriever later if needed.

@jslee02 jslee02 deleted the building_examples_tutorials branch February 14, 2017 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants