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

Update URDF parsing logic and improve R/W operations on files #100

Merged
merged 5 commits into from
Oct 25, 2024

Conversation

flferretti
Copy link
Contributor

@flferretti flferretti commented Oct 11, 2024

This PR updates the parsing logic for URDF files by bounding the check on file existence to the maximum path length allowed by the OS. Moreover, some changes were made to make the RW operations on files safer and some final linting was performed.

Closes ami-iit/comodo#21


📚 Documentation preview 📚: https://adam-docs--100.org.readthedocs.build/en/100/

Comment on lines +25 to +37
## Hack to remove the encoding urdf, see https://github.com/icub-tech-iit/ergocub-gazebo-simulations/issues/49
with open(model_path, "r", encoding="utf-8") as robot_file:
robot_urdf_string = (
robot_file.read()
.replace("<?xml", "")
.replace("version='1.0'", "")
.replace("encoding='UTF-8'?>", "")
)
Copy link
Member

Choose a reason for hiding this comment

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

@traversaro @CarlottaSartore did ros/urdf_parser_py#83 fix ros/urdf_parser_py#82? If yes, maybe these lines that remove the encoding can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so, but no release was done, so I guess the issue is still present in the latest release, and the problem is also that the pypi package is abandoned.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I see, so probably the solution for ADAM is to just migrate to something else, at one point in the future. Thanks for the feedback!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I guess we need to migrate soon :/

@diegoferigo diegoferigo requested a review from Giulero October 14, 2024 06:57
@Giulero
Copy link
Collaborator

Giulero commented Oct 24, 2024

Thanks a lot @flferretti! :)
I just left a comment related to the automatic format of a file. Automatic sorting of imports is fine. I should have inserted it before tbh :D

Comment on lines +25 to +37
## Hack to remove the encoding urdf, see https://github.com/icub-tech-iit/ergocub-gazebo-simulations/issues/49
with open(model_path, "r", encoding="utf-8") as robot_file:
robot_urdf_string = (
robot_file.read()
.replace("<?xml", "")
.replace("version='1.0'", "")
.replace("encoding='UTF-8'?>", "")
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I guess we need to migrate soon :/

]
},
{
"cell_type": "markdown",
"metadata": {
"id": "CZMO7PsmKUB6"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just selecting a few lines, but I'm considering the whole file. I was wondering: which is the automatic formatter you used? I will put it in the dependencies ;)
I would remove this file from this PR for now (just to enhance the clarity of this PR) and maybe add it in a new PR that just formats the files!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using black[jupyter] and isort for formatting.

Formatting removed in 79f6d67 with rebase

@flferretti
Copy link
Contributor Author

Thank you for the review @Giulero! I've addressed your comments. Before merging, please note that this PR is rebased onto #99

@Giulero Giulero merged commit f6dfb35 into ami-iit:main Oct 25, 2024
9 checks passed
@flferretti flferretti deleted the flferretti-patch-1 branch October 25, 2024 12:24
This was referenced Oct 29, 2024
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.

OSError when creating RobotModel
4 participants