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

VSK file parser #561

Merged
merged 14 commits into from
Dec 5, 2015
Merged

VSK file parser #561

merged 14 commits into from
Dec 5, 2015

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Nov 28, 2015

This pull request reinstates VSK parser that was obsoleted since DART 3.0.

The VSK file format, which is developed by Vicon Motion Systems, contains skeleton hierarchy and markers. The specifications of VSK structure can be found here. Note that the VSK file format lacks some information to build dynamic skeleton models such as body node's shape, mass, and inertia. The missing information can be provided through Options parameter to the parser.

Also, Parser.[h/cpp] files are renamed to more relevant name XmlHelpers.[h/cpp] with some improvements on parsing xml attributes.

- The attribute getters are defined for different types
- ElementEnumerator is templated to cover const and non-const elements
- The element value getters are changed to takes const element pointer
- Remove initMeshes() since it's not used anywhere
- Rename updateVolume() to computeVolume() to match the naming convention
- Add static methods computing volume and moments of inertia for shape whoes volume is not zero
@jslee02 jslee02 added this to the DART 6.0.0 milestone Nov 28, 2015
This was referenced Nov 28, 2015
@sehoonha
Copy link
Collaborator

sehoonha commented Dec 1, 2015

Tested on my data, and looks good to me.

# Resolved conflicts:
#	dart/dynamics/BoxShape.cpp
#	dart/dynamics/EllipsoidShape.cpp
#	dart/dynamics/MeshShape.cpp
@jslee02
Copy link
Member Author

jslee02 commented Dec 3, 2015

I believe this pull request is ready to merge. @mxgrey any comments?

@mxgrey
Copy link
Member

mxgrey commented Dec 3, 2015

I'll finish reviewing it tonight.

One thought that comes to mind is that the Marker class should probably be refactored into a Node once PR #531 is finished.

@psigen
Copy link
Collaborator

psigen commented Dec 3, 2015

@mxgrey: Agreed. It seems like much of what the Marker class was intended for is covered by the proposed functionality of the Node w.r.t. render geometry, right?

@jslee02
Copy link
Member Author

jslee02 commented Dec 3, 2015

Agreed to the idea.

@mxgrey
Copy link
Member

mxgrey commented Dec 4, 2015

Since VskParser doesn't have any internal state and its only member function is a static function, we may want to consider making VskParser a namespace instead of a class. Otherwise having it as a class sort of implies that there may be a situation where you'd want to create an instance of it.

@mxgrey
Copy link
Member

mxgrey commented Dec 4, 2015

I've finished looking through the pull request, and it generally looks good to me. I just think we should either change VskParser to a namespace or explicitly delete its constructor to make it clear that there's no use in creating an instance of the class.

@jslee02
Copy link
Member Author

jslee02 commented Dec 5, 2015

Updated this PR responding the comments. VskParser is changed to a namespace because it looks neater than explicitly deleting its constructor.

Will merge this PR once CI's are happy.

jslee02 added a commit that referenced this pull request Dec 5, 2015
@jslee02 jslee02 merged commit dca2755 into master Dec 5, 2015
@jslee02 jslee02 deleted the vsk_parser branch December 5, 2015 17:26
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.

None yet

4 participants