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

Fix convertShape to use getMeshPath #518

Merged
merged 8 commits into from
Apr 30, 2019
Merged

Fix convertShape to use getMeshPath #518

merged 8 commits into from
Apr 30, 2019

Conversation

gilwoolee
Copy link
Contributor

@gilwoolee gilwoolee commented Apr 17, 2019

This PR fixes rviz::convertShape(MeshShape...) method to use getMeshPath instead of getMeshUri for meshUri.
When a mesh is loaded via MeshShape, convertShape(MeshShape...) gets called. Since meshUri is used by Rviz, it has to be a raw filepath, which is not the case when using getMeshUri.

This was never an issue with Herb or Ada because we use urdfLoader and it constructs AssimpMesh, which doesn't call this function. It was discovered while @vinitha910 was trying to load cozmo mesh directly using raw filepaths.

It has been tested by constructing cozmo with MeshShapes and visualizing it in Rviz through aikido InteractiveViewer.


Before creating a pull request

  • Document new methods and classes
  • Format code with make format

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

@gilwoolee gilwoolee requested a review from brianhou as a code owner April 17, 2019 00:14
@gilwoolee gilwoolee requested a review from jslee02 April 17, 2019 00:14
@gilwoolee gilwoolee added this to the Aikido 0.3.0 milestone Apr 17, 2019
@gilwoolee gilwoolee requested a review from aditya-vk as a code owner April 17, 2019 00:17
Copy link
Contributor

@brianhou brianhou left a comment

Choose a reason for hiding this comment

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

LGTM, minor changes

jslee02
jslee02 previously approved these changes Apr 17, 2019
Copy link
Member

@jslee02 jslee02 left a comment

Choose a reason for hiding this comment

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

Looks good to me. The suggestions from @brianhou sound good to me as well.

@codecov
Copy link

codecov bot commented Apr 17, 2019

Codecov Report

Merging #518 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #518   +/-   ##
=======================================
  Coverage   75.59%   75.59%           
=======================================
  Files         243      243           
  Lines        5856     5856           
=======================================
  Hits         4427     4427           
  Misses       1429     1429

Co-Authored-By: gilwoolee <gilwoo301@gmail.com>
jslee02
jslee02 previously approved these changes Apr 17, 2019
@brianhou brianhou merged commit 694a846 into master Apr 30, 2019
@brianhou brianhou deleted the bugfix/rviz branch April 30, 2019 17:15
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.

3 participants