gazebosim / gazebosim/sdformat

Should clarify and document how/where include overrides are applied when using the Interface API

Open
#852 0 comments 0 reactions 1 assignee Claimed by @azeey View on GitHub
Dominant language
C++
Stars
216
Forks
125
Avg merge
1d 14h
Merged PRs (30d)
14

Description

## Description
Currently, when a model is included such that it's parsed by a custom parser via the Interface API, the expectation is that some of the include overrides are applied by the custom parser. This means, for example, `sdf::InterfaceModel::Name` would return the value of `//include/name` if it exists. However, other overrides, such as `//include/placement_frame` are retrieved from the `sdf::NestedInclude` data structure that is populated by libsdformat before being sent to the custom parser. This is because `sdf::InterfaceModel` does not hold placement frame information. This is especially ambiguous for `//include/pose` because `sdf::InterfaceModel` only has `sdf::InterfaceModel::ModelFramePoseInParentFrame`, which does not include the information contained in the `//include/pose[@relative_to]` attribute. Therefore, the code in `FrameSemantics.cc` uses the information in `//include/pose` if it's present.

## Proposed behavior ideas:

1. Do not expect parsers to look at `//include` info. This is not practical because parameters like `name` and `static` can affect the behavior of the parser.

2. Assume custom parser has applied all overrides, so only take information from `sdf::InterfaceModel`. However, not all the information that might be contained in `//include` is available from `sdf::InterfaceModel`

3. Hybrid, which is the current state of things:
1. _Option A_: `sdf::InterfaceModel` is the source of truth for all parameters that can be overriden and are available through `sdf::InterfaceModel`. Currently, these are `name` and `static`
_Option B:_ The curstom parser should consume the information in `//include` and apply the override internally, but libsdformat won't trust that the overrides have been applied, so it will use `sdf::NestedInclude` as the source of truth.
2. `sdf::NestedInclude` is the source of truth for all override parameters that may be contained `//include`, but not available through `sdf::InterfaceModel`. `//include/pose` will be treated as one of these because `sdf::InterfaceModel::ModelFramePoseInParentFrame` does not have sufficient information to capture what can be expressed in `//include/pose`.

(1) and (2) are not viable. I lean toward (3) with option A.

/cc @EricCousineau-TRI

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.