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 skeleton import functionality to mesh_loader #1654

Merged
merged 3 commits into from
Sep 15, 2021

Conversation

kar-re
Copy link
Contributor

@kar-re kar-re commented Aug 18, 2021

Description

This adds the functionality of importing an animated mesh to RViz using the same resource_retriever as in mesh_loader.

Due to the way Ogre3D loads skeleton meshes a developer can automatically import an animated mesh using the same loadMeshFromResource method already present, but with the added functionality of assigning animation states.
The method is mostly the same as loadMeshFromResource but with changes to load the .skeleton file as well into the default resource group.

Happy to accept any constructive critiques or suggestions on how to make it better!

Checklist

  • If you are addressing rendering issues, please provide:
    • Images of both, broken and fixed renderings.
    • Source code to reproduce the issue, e.g. a YAML or rosbag file with a MarkerArray msg.
  • If you are changing GUI, please include screenshots showing how things looked before and after.
  • Choose the proper target branch: latest release branch, for non-ABI-breaking changes, future release branch otherwise.
    Due to the lack of active maintainers, we cannot provide support for older release branches anymore.
  • [Will do if accepted! ] Did you change how RViz works? Added new functionality? Do not forget to update the tutorials and/or documentation on the ROS wiki
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers of RViz. Refer to the RViz Wiki for reviewing guidelines.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I don't yet see the use of this PR. Could you explain more, please?
According to the OGRE doc, skeleton definitions are loaded on demand, especially when referenced by a Mesh. So, do we really need to explicitly load them?

How do you intend animating a mesh based on the skeleton? The SkeletonPtr isn't used yet.

src/rviz/mesh_loader.cpp Outdated Show resolved Hide resolved
@kar-re
Copy link
Contributor Author

kar-re commented Aug 26, 2021

Hi @rhaschke!

Yes, sorry for not providing enough of a background.

So, just as you say Ogre3D loads the skeleton definition on demand when being referenced by a mesh. Unfortunately due to the package://-URI Ogre3D has trouble autoloading the skeleton, and this PR remedies that issue by loading the .skeleton file if present, and then Ogre3D can find it in the default resource group.

Let me show this more clearly by using an example:
Say a developer is following this tutorial for using animated meshes in Rviz, and the developer knows it's using Ogre3D for rendering:
https://wiki.ogre3d.org/Intermediate+Tutorial+1

Then the developer will be met with the following error:
[ WARN] [1629966114.309848800]: OGRE EXCEPTION(6:FileNotFoundException): Cannot locate resource XXXX.skeleton in resource group General or any other group. in ResourceGroupManager::openResource at /build/ogre-1.9-kiU5_5/ogre-1.9-1.9.0+dfsg1/OgreMain/src/OgreResourceGroupManager.cpp (line 753)

And if the developer tries setting an animation state which isn't present Rviz will crash.
The effort to remedy this includes looking through the mesh_loader in order to see how RViz handles mesh loading, and replicating this in the same way that this PR does.

This PR therefore remedies this by checking if there's an .skeleton mesh present, loading that into the default resource group, and assigning it to the mesh.

So in summary, a small change, which makes the developer experience more seamless.

The reason I kept loadSkeletonFromResource public is to enable others to manually assign other skeletons to meshes, if that is wanted. For example the _notifySkeleton method.

@rhaschke
Copy link
Contributor

Thanks for the clarification.
I guess you are using the skeleton feature from a custom display then as rviz doesn't provide support for this (yet).

@rhaschke
Copy link
Contributor

Closing and reopening to re-trigger CI.

@rhaschke rhaschke closed this Aug 26, 2021
@rhaschke rhaschke reopened this Aug 26, 2021
src/rviz/mesh_loader.cpp Outdated Show resolved Hide resolved
@kar-re
Copy link
Contributor Author

kar-re commented Sep 15, 2021

Hi @rhaschke, just wanted to say thanks for implementing the code review change, and sorry for the late response. Please let me know if there's anything else I can do! :)

@rhaschke rhaschke merged commit 40af0bb into ros-visualization:noetic-devel Sep 15, 2021
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.

2 participants