o3de / sig-simulation

Special Interest Group for Simulation
Apache License 2.0
10 stars 14 forks source link

Proposed RFC Feature - Option to disable inheriting parent's transform at runtime #57

Closed greerdv closed 1 year ago

greerdv commented 1 year ago

Option to disable inheriting parent's transform at runtime

Summary

A setting would be added to the transform component which allows turning off the inheritance of transform information from a parent (at runtime; the inheritance behaviour will still be desirable at edit time). The setting would be available via the transform interface, and would allow other components to switch off that behaviour if they will be responsible for controlling an entity's transform.

What is the relevance of this feature?

Certain components are intended to control the transform of an entity, such as:

If an entity is a child of another entity, then its transform will also update when the parent moves, and the two transform updates can end up fighting. This leads to a couple of problems:

Feature design description

Technical design description

A setting will be added to the transform component, accessible via the transform interface. This proposal does not include exposing that setting on the transform component's component card, but that could be done as a separate, future proposal.

What are the advantages of the feature?

It allows objects which may be represented as multiple entities (e.g. vehicles, articulations) to have a representation in the entity hierarchy which reflects their logical organization.

What are the disadvantages of the feature?

Currently there is a very simple rule for how the hierarchy affects an entity's transform. This proposal could make it harder to reason about expected transform behaviour.

Are there any alternatives to this feature?

How will users learn this feature?

The intent is that the feature will be largely invisible to most users. For specific use cases such as articulations, it could be communicated through tutorials or demo content.

Are there any open questions?

greerdv commented 1 year ago

Note that this RFC is a continuation of https://github.com/o3de/o3de/discussions/14416. See that discussion for some important existing comments.

lmbr-pip commented 1 year ago

Thanks for writing this. Had three quick comments

  1. Can you add an explicit statement that the default behaviour is to have the option off, so its clear theres no behaviour change unless you opt-in? IMHO its implied but should be stated.
  2. "Runtime" is a loose a concept in O3DE, as we have "play-in-the-editor" runtime versus explicit launcher runtimes. I would ensure both are called out here.
  3. I would argue that because we are adding a behaviour that could change at runtime, that it needs to be visible in the entity outliner or similar. I can imagine the frustration of debugging some strange networked entity hierarchy issues only to discover some unexpected code (script, component etc) has turned it on somewhere in a complex hierarchy. Having some visual reminder of this is probably important to drive discovery and aid collaborative building.
greerdv commented 1 year ago

@lmbr-pip I'll update the document shortly but just to reply to your comments:

  1. The default behaviour would be for transforms to be inherited from parents. However, I now realize I didn't make something else clear in the RFC, which is that the intent would be for a small set of other components (I'll use rigid body as an example here and below) to use the transform interface to be able to turn that behaviour off at runtime. This shouldn't break anything (famous last words!) because previously it would have been impossible to use e.g. a rigid body on a child without getting broken behaviour. And rigid bodies on non-child entities would not be affected by the change.
  2. I would still consider playing in the editor to be runtime, because you create runtime entities with runtime components. I don't think that's particularly unclear, but I can add a clarification just in case.
  3. I did go back and forth on whether it was a good idea to expose the setting, and I'm still leaning towards not doing it this time and making it a potential future change, although I am of course open to discussion on that point and I know at least one other person who favours exposing it. One concern I have is that exposing it would be a very difficult thing to undo, and is not required to fix the problems which motivated the RFC. Another is that some careful UX thought would be required to handle communicating the fact that other components may want to override what the user has chosen.
adamdbrw commented 1 year ago

@greerdv I looked into the proposal and I agree with it.

Open question: could we detect situations where the user probably doesn't know / tries to do something which could be undefined or makes no sense - and warn them somehow?

moraaar commented 1 year ago

As I commented in the discussion I support this RFC. I think it'd be useful if the entity outliner would visualize differently the entities that won't inherit the transform from their parents, this can help understanding the behavior of the entities.

lgleim commented 1 year ago

Since implementation of RFC can improve both user and developer experience for use cases, such as robotics, I generally support this RFC.

Nevertheless, after reading up on the discussion up to this point, I am wondering whether the primary question here may rather be who or what controls the transform at runtime then if an entity transform is relative to its parent or not. Generally, I believe that a single controller should own the transform, to avoid "controller fighting". To reframe transform inheritance in light of this question, consider entity transform control to be either passive, (uncontrolled,) or active:

In that view, the entity hierarchy in the Editor is a logical hierarchy first. At runtime, the default passive control strategy would (logically) convert this logical hierarchy to a transform hierarchy, (uncontrolled entities would not set up any transform update mechanism,) while active controllers may implement their own strategy that may or may not rely on transform inheritance following the editor hierarchy.

To me, this framing seems more intuitive from a user perspective than reasoning over transform hierarchies. Instead of expose any additional configuration flags to end-users (cf. Method 3), a user's only choice is whether to add an active transform controller (e.g. Rigid Body Component) or not, as they are already doing. Disabling transform inheritance is then an internal implementation detail (cf. Method 2), largely irrelevant to the user. To raise user awareness of the internal mechanism, a corresponding control status may be exposed in the UI of the transform component, e.g. as uncontrolled, relative to parent or controlled by 'rigid body component' etc.

Having entities with uncontrolled transform or only active and passive control is equivalent to the decision between Method 2 & 3. So in the end, my argumentation is mainly about how to expose this change to users and how to avoid the motivating issue of "controller fighting" in the future.

Independent of this question, two points from the previous discussion seem worthwhile raising here again:

lgleim commented 1 year ago

Discussed in today's governance meeting:

greerdv commented 1 year ago

@lgleim I like the idea of introducing a concept along the lines of transform controller, and only allowing one controller to be active at any one time. I would suggest making that into a separate RFC though for a couple of reasons:

Whether a component is controlling the transform can change at runtime (for example if you call DisablePhysics on a rigid body component, it would stop controlling the transform), so it might not be possible to determine at editor time which component has control. I think the best we can do at editor time might be a visual indication that an entity has 1 on more components which might control the transform (depending on their state at runtime).

lgleim commented 1 year ago

Discussed in today's issue triage and moved into final comment period until next week's triage. As this RFC is moving closer towards acceptance, additional issues/RFCs should be created for identified directions for followup work. @greerdv @SergeyAMZN could you take a look at this?

hultonha commented 1 year ago

The final comment period has now expired and there have been no objections or change suggestions in the last week. I propose we now close this issue and continue with the implementation (discussed in sig-simulation triage). Could you please confirm @lgleim and close the issue as complete if you agree? Thank you!