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

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

Closed
greerdv opened this issue Feb 20, 2023 · 10 comments

Comments

@greerdv
Copy link
Contributor

greerdv commented Feb 20, 2023

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:

  • Rigid body component
  • Actor component
  • Attachment component (a component which connects an entity to a joint on an actor e.g. so you can pick up an item and have it move with the character's hand)

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:

  • If components like the ones mentioned above are on a child entity, it will likely lead to unexpected behaviour, the cause of which is not obvious, hard to diagnose and generally not documented.
  • The current easiest way to avoid problems is to only put entities with affected components at root level (or at least not children of moving parents), but that itself has some problems:
    • There's no guidance at the moment to know that's what you need to do, either in documentation or UI.
    • It puts a restriction on how you organize your entities. There are a lot of situations where it's natural from an organizational perspective to make entities children (e.g. wheels as child entities in a vehicle). Forcing those entities to live at root level could also lead to very flat hierarchies in the entity outliner which are hard to work with because they don't have a nice hierarchical structure.
    • Certain workflows are awkward, for example authoring a PhysX joint, because the follower cannot be a child of the lead, which means that it does not move with the lead in the editor.

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?

  • Stick with the current inheritance behaviour
    • For physical hierarchies such as articulations or vehicles, users would have to continue authoring as flat entity hierarchies which do not reflect the logical organization.
    • If we did go for this approach, we ought to add improved documentation on hierarchy patterns to avoid or automatically disable affected components and issue a warning if problematic hierarchies are detected.

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
Copy link
Contributor Author

greerdv commented Feb 20, 2023

Note that this RFC is a continuation of o3de/o3de#14416. See that discussion for some important existing comments.

@lmbr-pip
Copy link

lmbr-pip commented Feb 20, 2023

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
Copy link
Contributor Author

greerdv commented Feb 20, 2023

@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
Copy link
Contributor

adamdbrw commented Feb 21, 2023

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

  • This is probably the most reasonable change to support articulations, which we definitely need.
  • I think that visualizing this different setting for the entity for the user is important (to avoid having "mysterious" effects). This does not mean the setting needs to be exposed and I am for keeping it only in the transform interface as proposed. How to best communicate the information that this entity/hierarchy now works differently?
  • As an opt-in, this is relatively conservative approach, keeping the default behavior as is and only affecting users who decide to turn it on (hopefully).

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
Copy link
Contributor

moraaar commented Feb 21, 2023

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
Copy link
Contributor

lgleim commented Feb 23, 2023

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:

  • Passive control (default) would be equivalent to the current transform hierarchy, automatically handling transforms relative to the parent.
  • (Uncontrolled would mean that that transform inheritance is disabled, effectively rendering the entity static.)
  • Active control would disable passive control in the form of transform inheritance for the entity (cf. Method 2) and transfer control to some active control system (think e.g. the Rigid Body Component)

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:

  • Does this change affect the hierarchy system in Multiplayer gem? E.g., would this require changes to the (delta-based) transform replication mechanism for hierarchies? Discussed in today's governance meeting. Should not cause downstream issues.
  • Should only a single component have control over the entity transform at any time? The transform component could then also report in the Editor UI which component controls it at runtime?

@lgleim
Copy link
Contributor

lgleim commented Feb 23, 2023

Discussed in today's governance meeting:

  • If taken further, the transform inheritance setting should not be exposed at first to allow for longer-term testing of implications and potential roll-back without confusing users.

@greerdv
Copy link
Contributor Author

greerdv commented Mar 1, 2023

@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:

  • it adds a bit more complexity
  • I would argue that it it's more of a long term nice to have thing, whereas the proposal in this RFC has an immediate benefit for implementing articulations

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
Copy link
Contributor

lgleim commented Mar 2, 2023

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
Copy link
Collaborator

hultonha commented Mar 9, 2023

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!

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

6 participants