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

remove up_axis tag from DAE meshes #69

Closed
wants to merge 1 commit into from

Conversation

T045T
Copy link

@T045T T045T commented Dec 12, 2017

RViz doesn't respect it anyway ( ros-visualization/rviz#1045 ), but current versions of three.js' ColladaLoader (used in the RobotWebTools, see RobotWebTools/ros3djs#202 ) do!

(Since RViz doesn't respect the tag, this does not break the model)

RViz doesn't respect it anyway ( ros-visualization/rviz#1045 ), but current versions of three.js' ColladaLoader (used in the RobotWebTools, see RobotWebTools/ros3djs#202 ) do!

(Since RViz doesn't respect the tag, this does not break the model)
@rctoris
Copy link
Contributor

rctoris commented Dec 14, 2017

I went through the chain of tickets that lead to this PR and think I have an understanding. But I guess my initial confusion is, is this a problem with ThreeJS, or is ThreeJS correct and our model is wrong but we just don't notice because RViz doesn't care?

@T045T
Copy link
Author

T045T commented Dec 14, 2017

It's the latter - ThreeJS correctly applies the up_axis tag to models now (the older version ros3djs used before did not).

RViz doesn't check for up_axis and probably won't change because that would break some models.

As such, we'll probably modify ThreeJS' ColladaLoader downstream (or petition them to add an option to ignore up_axis) to keep ros3djs in sync with RViz' behavior.

However, I figured running a one line shell command on the repo to clean things up wouldn't hurt, even if it does not immediately change anything :)

If anything, this patch might prevent some confusion when loading the models in a non-RViz, non-ros3djs tool that does respect up_axis

@mikeferguson
Copy link
Contributor

What about Gazebo? The loader there is different than the loader in RVIZ, has this been tested in Gazebo?

@T045T
Copy link
Author

T045T commented Dec 14, 2017

that's a very good question - I'll check just to make sure, but if Gazebo handled things differently from RViz, the model would have been broken in one of the two, but not the other, before this change.

@moriarty
Copy link
Contributor

¯\(ツ)/¯ I don't have much opinion on this... @rctoris I don't think it breaks anything, and can probably be merged.

Actually digging into the linked issues I found these:

  1. not loading robot model from srdf personalrobotics/or_urdf#33
  2. How to access the head camera? jontromanab/fetchpy#2

Which are both Fetch specific and relate back to this Issue.
@jontromanab would this PR fix the above issues?

Also, for non Fetch related PRs similar to this one, some have been merged:

@jontromanab
Copy link

jontromanab commented Jul 27, 2018

Hello @moriarty. My issue was a little bit different in the sense that Assimp, the openrave mesh handler, and Rviz handles meshes little differently. Making the y-axis up generally solves the issues in openrave. That's what my code in process_urdf does. It may work in Rviz and three.js, but I am highly doubtful, it may work for other systems such as openrave.

@T045T
Copy link
Author

T045T commented Jul 28, 2018

Yes, Y_UP is the default, I believe - so removing the up_axis tag altogether or changing it to Y_UP should not make a difference :)

@moriarty moriarty changed the base branch from indigo-devel to melodic-devel August 6, 2019 21:34
@moriarty
Copy link
Contributor

moriarty commented Aug 6, 2019

Opps, I used the GitHub interface to change this PR to go into melodic-devel and I don't think it worked, there are now many conflicts, I'll try to undo that.

@moriarty moriarty changed the base branch from melodic-devel to indigo-devel August 6, 2019 21:36
@moriarty
Copy link
Contributor

moriarty commented Aug 6, 2019

#105 changed this to

-<up_axis>Z_UP</up_axis>
+<up_axis>Y_UP</up_axis>

Yes, Y_UP is the default, I believe - so removing the up_axis tag altogether or changing it to Y_UP should not make a difference :)

So I'll close this, it's fixed in melodic-devel

@moriarty moriarty closed this Aug 6, 2019
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