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

Refactor urdf parser function #94

Merged
merged 5 commits into from Jul 27, 2023

Conversation

quarkytale
Copy link
Collaborator

Making init_collada and init_urdf consistent with new changes in init_sdf.
Changes:

  • refactored joint tag loop
  • updated limit checks

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
if jtype in ['fixed', 'floating', 'planar']:
continue
name = child.getAttribute('name')
self.joint_list.append(name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this should move to right before the if name in dependent_joints check. That way we avoid a situation where we could add it to self.joint_list, but then fail to add it to the free_joints because of a missing limit.

limit = joint.getElementsByTagName('limits')[0]
if joint:
limit_list = joint.getElementsByTagName('limits')
if limit_list:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is again changing the semantics slightly. I think here we should probably do something more like:

Suggested change
if limit_list:
if not limit_list:
continue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

The more I look at it, the more I want to clean up init_collada here to be more robust. In particular, it is assuming a lot of things about the passed-in COLLADA file, and some of those things just may not be true. I've been looking in particular at https://www.khronos.org/files/collada_spec_1_5.pdf .

So what I'm going to suggest here is that we make this PR just about refactoring init_urdf, which is an easier problem. Once we have that in, we can do another followup PR where we refactor init_collada so it is much more robust. @quarkytale does that make sense?

@quarkytale quarkytale changed the title Refactor urdf and collada parser functions Refactor urdf parser function Jul 26, 2023
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale
Copy link
Collaborator Author

Alright this should just have init_urdf changes now, will open up a separate one for init_collada once this and init_sdf is in!

Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A few more things to fix, this is getting closer.

Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One more small change, then this will be good to go.

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks great to me. Thank you for this cleanup!

@clalancette clalancette merged commit 47a47d6 into ros:ros2 Jul 27, 2023
4 checks passed
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

2 participants