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 functionality to create a robot model from xml string rather than filepath #200

Open
bpapaspyros opened this issue Oct 17, 2024 · 2 comments
Assignees

Comments

@bpapaspyros
Copy link
Member

Motivation:

This comes up in various occasions where we have a URDF as a string (instead of its path) and want to create a Model but no viable constructor option exists.

Instead, what we are currently doing is support loading from a file path. Consequently, if we are in need of a Model instance and have the URDF as a string from a different source, we would have to create a file temporarily and create the model instance with that file path.

That is not ideal, but it is a fair design choice. Having said that, the control-libraries do not internally require the file path again.

Suggestion

One way to support loading from strings would be to attempt creating the model by first assuming it's an XML string, and upon failure we treat it as a file path.

The advantage is that there is minimal changes to be made, but it's not a very clean solution since get_urdf_path will now have to return an empty std::string or std::optional<std::string> (I suggest the latter) passing the responsibility downwards.

std::optional<const std::string> get_urdf_path() const;
// std::optional<const std::string&> get_urdf_path() const;  // C++20, not a big problem if we don't have the reference

Which I think is sensible.

I would then suggest that some of the member variables are updated to reflect that change. For example:

urdf_path_ 

can now be

urdf_ 
@bpapaspyros bpapaspyros self-assigned this Oct 17, 2024
@bpapaspyros
Copy link
Member Author

@domire8 @eeberhard let me know what you think

@domire8
Copy link
Member

domire8 commented Oct 18, 2024

Thanks, yes I agree that both options should be supported. However, the suggested change is breaking (might be wrong, you could double check that by building local versions). So if we break the robot model class, we might as well look for a bigger improvement and then we can get rid of get_urdf_path entirely and stick with get_urdf or so. I just don't know how we can tackle this reasonably given that we are likely to break everything.

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

No branches or pull requests

2 participants