Pass arguments to SDF files #5
Conversation
Signed-off-by: osrf <[email protected]>
Signed-off-by: osrf <[email protected]>
|
|
||
| ## Implementation | ||
|
|
||
| The initial question was whether this process should be a [SDF](http://sdformat.org/) or [ign-gazebo](https://github.com/ignitionrobotics/ign-gazebo) feature. The conclusion is that this will most likely be implemented in ign-gazebo. The reasoning behind this is other organizations use SDF with other simulators, which raises concerns that it could affect their work. Also, this desired argument passing process is intended for ign-gazebo use. Although there may be minor updates to SDFormat and/or libsdformat, the current plan is to implement the main functionality in ign-gazebo. Below describes the current idea to design this new feature. |
There was a problem hiding this comment.
The reasoning behind this is other organizations use SDF with other simulators, which raises concerns that it could affect their work.
I understand the concern, writing something generic is always more complicated than writing something specific. But in this case, I think that functionality like adding / removing / modifying included elements could be considered general enough to be supported directly into SDFormat.
The part about using ERB to set parameters, on the other hand, would be too disruptive to add to SDF, introducing a large, and often complicated dependency (Ruby). So I agree that this part should be implemented at the Gazebo / Launch level.
There was a problem hiding this comment.
anything we add to the SDFormat specification increases its complexity, so we have to be judicious about what becomes part of the official spec. I think it is possible that this type of functionality eventually makes its way into the official spec, but I think we should start by using custom elements parsed by ign-gazebo as you propose here (it could be elegant to create a plugin system for libsdformat that would parse custom elements, but that would take some additional design work). If after demonstrating the feature in ign-gazebo, if there is broader interest in the feature, we can propose adding it to the official spec.
I also agree that ERB support shouldn't be part of the official spec
There was a problem hiding this comment.
anything we add to the SDFormat specification increases its complexity
That's a good point, whatever is in the spec needs to the supported by all parsers, not only libsdformat.
I'd argue that the usefulness of this feature could be worth the added complexity though. See how many issues could be fixed by this:
- override arbitrary parts of included models sdformat#128
- Allow physics presets to be <include>'ed sdformat#87
- Allow inclusion of links, collisions, and visuals sdformat#75
after demonstrating the feature in ign-gazebo, if there is broader interest in the feature, we can propose adding it to the official spec
Some concerns I have with making this ign-gazebo specific at first:
- Other libraries like
ign-physicsandign-sensorsalso support SDF, so we'd need to be careful not to pass those gazebo-specific elements to those libraries. - Users will start writing Gazebo-specific SDF files which aren't reusable by other simulators. This decreases the advantages of using a standard spec.
- If we decide to move this to the official spec after implementing into Gazebo, we'll need to go through a deprecation cycle of all SDF files written using the Gazebo spec. Or we'll need to write converters.
There was a problem hiding this comment.
FWIW, Drake has a very similar pain point with SDFormat, and we would be eager to take inspiration from whatever solutions (or specification changes) you come up with, albeit at our usual somewhat slower pace.
There was a problem hiding this comment.
Some concerns I have with making this
ign-gazebospecific at first:
those concerns make sense to me. In that case, I would target some of these extensions of the <include> element at the SDFormat 1.8 composition proposal:
Signed-off-by: osrf <[email protected]>
|
I'm just starting to read through this, and I've noticed a few places where SDF is used for both the specification and the reference parsing library; we're trying to use SDFormat to refer to the specification and Does this terminology make sense? |
| * `modify`: indicates that the values in the original model are to be modified to the new listed values | ||
| * `replace`: replaces the elements and/or values from the original model to the new provided elements and/or values | ||
| * `add`: adds new elements to the original model | ||
| * `remove`: removes (or disables) the elements from the original model |
There was a problem hiding this comment.
seeing these actions listed out reminds me of the functionality of the sdf::Converter class (see also sdf/1.6/1_5.convert and sdf/1.7/1_6.convert). it's currently internal to libsdformat, so you couldn't call it as an API, but the code itself could be useful to refer to or to fork into your own implementation
There was a problem hiding this comment.
The same could be done using the Element class correct (e.g., AddElement, RemoveChild, Set)?
I guess it comes down to if we want to implement this in ign-gazebo or in libsdformat? If we implement this in ign-gazebo, then libsdformat's Element class could be used. If in libsdformat, then some new function (e.g., LoadNewArgs) in the Root class could use sdf::Converter but this approach may be tedious/confusing to the user when creating the SDF.
There was a problem hiding this comment.
sure, the existing Element APIs could be used; I just meant to say that the Converter class is already doing something similar, so it would be a shame to have to maintain two different approaches to doing the same type of xml transformation. If it's feasible to share code, I would modify the sdf::Converter class to support that, but they can also remain separate if it doesn't make sense to share code
|
|
||
| In a launch/world file, the user can construct a model using model composition and include components of the model using the `<include>` tag. If a user wants to modify parameters of the included model, then the user will use the custom element `<gazebo:params>` that will contain the adjusted elements. In the custom element `<gazebo:params>`, `gazebo` is the namespace prefix and `params` is the custom element. This approach was chosen so that downstream users can choose to ignore the namespaced custom element and its contents. The included model (not the constructed model in the launch/world file) will be referred to as the original model/file. In the launch/world file and under `<include>`, the elements listed under `<gazebo:params>` will indicate elements from the original model and will specify new values and/or elements to be updated, added, and/or removed. | ||
|
|
||
| To specify an element from the original file, the tag (e.g., `<link>`, `<sensor>`, `<visual>`) needs to be provided as well as the name attribute but the name is the name of all the parent elements including its own name separated by double colon (`::`) syntax (e.g., `name="chassis::camera::cam"` where "chassis" is the name of the `<link>`, "camera" is the name of the `<sensor>`, and "cam" is the name of the `<camera>`). This is how the correct element will be identified and will be called the element identifier. Although it is possible to identify the element without requiring the user to specify the tag, enforcing the user to provide the tag allows less ambiguity to the user as well as provides a means of error checking. Then the user will need to specify a corresponding action with that element to dictate the alteration they would like to make using a custom attribute `action`. This attribute takes in a string and the available actions at the specified element are: |
|
|
||
| ### Parameter Specification | ||
|
|
||
| Parameters will be specified by the user in either launch (`.ign`) or world (`.sdf`) files through the use of model [composition (or nested models)](http://sdformat.org/tutorials?tut=composition) which will contain `<include>` tags and [custom elements](http://sdformat.org/tutorials?tut=custom_elements_attributes_proposal&cat=pose_semantics_docs). Model composition provides a way to create a model by nesting other models within itself. Discussed in this section is the proposed design for specifying parameters and the actions available to the user. The intent is to provide a means of manipulating almost all parameters in SDF models through launch/world files without requiring modifications to already existing/created model files. There is a limitation with this proposed approach with specifying plugins since plugins may not have unique names. This will be elaborated later on as well as a potential method of resolving this will be discussed, which may require a user to slightly modify the original model. |
There was a problem hiding this comment.
There is a limitation with this proposed approach with specifying plugins since plugins may not have unique names.
I think this is actually violation of the SDFormat 1.7 spec by ign-gazebo, since the SDFormat 1.7 spec requires sibling elements of any type to have unique names. I believe ign-gazebo is currently specifying plugin class types using the name attribute, but it would be better to specify them with a custom namespaced attribute like:
<plugin name='unique_name' filename='...' ignition:class='ignition::gazebo::systems::SystemName' />
There was a problem hiding this comment.
reported in gazebosim/gz-sim#399
| <include> | ||
| <uri>model://base_robot</uri> | ||
|
|
||
| <gazebo:params> |
There was a problem hiding this comment.
| <gazebo:params> | |
| <sdf:params> |
|
|
||
| Parameters will be specified by the user in either launch (`.ign`) or world (`.sdf`) files through the use of model [composition (or nested models)](http://sdformat.org/tutorials?tut=composition) which will contain `<include>` tags and [custom elements](http://sdformat.org/tutorials?tut=custom_elements_attributes_proposal&cat=pose_semantics_docs). Model composition provides a way to create a model by nesting other models within itself. Discussed in this section is the proposed design for specifying parameters and the actions available to the user. The intent is to provide a means of manipulating almost all parameters in SDF models through launch/world files without requiring modifications to already existing/created model files. There is a limitation with this proposed approach with specifying plugins since plugins may not have unique names. This will be elaborated later on as well as a potential method of resolving this will be discussed, which may require a user to slightly modify the original model. | ||
|
|
||
| In a launch/world file, the user can construct a model using model composition and include components of the model using the `<include>` tag. If a user wants to modify parameters of the included model, then the user will use the custom element `<gazebo:params>` that will contain the adjusted elements. In the custom element `<gazebo:params>`, `gazebo` is the namespace prefix and `params` is the custom element. This approach was chosen so that downstream users can choose to ignore the namespaced custom element and its contents. The included model (not the constructed model in the launch/world file) will be referred to as the original model/file. In the launch/world file and under `<include>`, the elements listed under `<gazebo:params>` will indicate elements from the original model and will specify new values and/or elements to be updated, added, and/or removed. |
There was a problem hiding this comment.
Should check tinyxml2, which might not support XML namespaces. Also, can you change the namespace from gazebo to sdf?
There was a problem hiding this comment.
I think we won't need namespaces elements if this becomes part of the official spec. Regarding tinyxml2, I don't think it fully supports namespaces, but our plan has been to ignore any elements or attributes that contain a single : character, which should be enough to allow them to pass through.
|
|
||
| In a launch/world file, the user can construct a model using model composition and include components of the model using the `<include>` tag. If a user wants to modify parameters of the included model, then the user will use the custom element `<gazebo:params>` that will contain the adjusted elements. In the custom element `<gazebo:params>`, `gazebo` is the namespace prefix and `params` is the custom element. This approach was chosen so that downstream users can choose to ignore the namespaced custom element and its contents. The included model (not the constructed model in the launch/world file) will be referred to as the original model/file. In the launch/world file and under `<include>`, the elements listed under `<gazebo:params>` will indicate elements from the original model and will specify new values and/or elements to be updated, added, and/or removed. | ||
|
|
||
| To specify an element from the original file, the tag (e.g., `<link>`, `<sensor>`, `<visual>`) needs to be provided as well as the name attribute but the name is the name of all the parent elements including its own name separated by double colon (`::`) syntax (e.g., `name="chassis::camera::cam"` where "chassis" is the name of the `<link>`, "camera" is the name of the `<sensor>`, and "cam" is the name of the `<camera>`). This is how the correct element will be identified and will be called the element identifier. Although it is possible to identify the element without requiring the user to specify the tag, enforcing the user to provide the tag allows less ambiguity to the user as well as provides a means of error checking. Then the user will need to specify a corresponding action with that element to dictate the alteration they would like to make using a custom attribute `action`. This attribute takes in a string and the available actions at the specified element are: |
There was a problem hiding this comment.
Can you add a paragraph about error conditions. Here are some:
- What happens when an element is referenced incorrectly, or the element doesn't exist?
- What happens when a change violates the SDF spec?
|
|
||
| In a launch/world file, the user can construct a model using model composition and include components of the model using the `<include>` tag. If a user wants to modify parameters of the included model, then the user will use the custom element `<gazebo:params>` that will contain the adjusted elements. In the custom element `<gazebo:params>`, `gazebo` is the namespace prefix and `params` is the custom element. This approach was chosen so that downstream users can choose to ignore the namespaced custom element and its contents. The included model (not the constructed model in the launch/world file) will be referred to as the original model/file. In the launch/world file and under `<include>`, the elements listed under `<gazebo:params>` will indicate elements from the original model and will specify new values and/or elements to be updated, added, and/or removed. | ||
|
|
||
| To specify an element from the original file, the tag (e.g., `<link>`, `<sensor>`, `<visual>`) needs to be provided as well as the name attribute but the name is the name of all the parent elements including its own name separated by double colon (`::`) syntax (e.g., `name="chassis::camera::cam"` where "chassis" is the name of the `<link>`, "camera" is the name of the `<sensor>`, and "cam" is the name of the `<camera>`). This is how the correct element will be identified and will be called the element identifier. Although it is possible to identify the element without requiring the user to specify the tag, enforcing the user to provide the tag allows less ambiguity to the user as well as provides a means of error checking. Then the user will need to specify a corresponding action with that element to dictate the alteration they would like to make using a custom attribute `action`. This attribute takes in a string and the available actions at the specified element are: |
There was a problem hiding this comment.
Is it possible to pass parameters to nested models?
|
Spoke with @scpeters and @azeey , since the consensus is to implement this in SDFormat, I will be working on a proposal to submit to sdformat.org. Once complete, I will update this design proposal to refer to the SDFormat proposal and add context/missing elements for the ignition portion. |
|
New proposal submitted to SDFormat: gazebosim/sdf_tutorials#46 |
|
Closing PR since the design and implementation only requires SDFormat. |
Initial draft is ready for feedback.
CC @nkoenig @scpeters @chapulina