ISXB-512, ISXB-521 Fix TrackedPoseDriver serialization migration

[chris-massie]

Description

Fixes an issue where the bindings were cleared due to invalid serialization migration logic in some cases.

• ISXB-512 TrackedPoseDriver bindings disappear when modifying an XR Origin Prefab
• ISXB-521 Tracked Pose Driver (Input System) bindings on a prefab become lost upon exiting editing the Prefab

Changes made

Modified the serialization logic so it won't replace the newer InputActionProperties if either the reference or embedded input action are already set.

I also made some other fixes I discovered while fixing the above:
Currently, the IDs of Input Actions are generated automatically when calling the id property or when the InputAction property drawer draws the field. However, if you create a GameObject and add a component with one serialized through API, the m_Id field wouldn't be generated in the serialized asset. This was causing the GameObject > XR > XR Origin (VR) rig created to not have those fields set since it was on a child GameObject. If a user dragged the root to create a prefab, when they would click on the child GameObject in the prefab or in the Hierarchy window, it would create a prefab override with new generated IDs. This change makes the constructor generate an ID so that it doesn't need to be visible in the Inspector window to be generated.

Also fixed the Clone methods to have it copy the m_Flags field so that the Initial State Check checkbox is copied.

Notes

This fix still does not fix the issue of nested prefabs where the parent prefab has an override to add a new input binding. The base prefab will already have the InputActionProperty upgraded, so the modified old deprecated field with the additional singleton bindings doesn't get copied over to the new property. There doesn't seem to be a way to reliably detect this situation. However, this is already the case with the old migration method, so it doesn't make it worse.

It's still possible for a prefab instance to have a noisy prefab override of the action name since the name, like the ID, is generated by the property drawer. However, if you create the InputAction with a non-empty name argument, it won't have that issue.

Checklist

Before review:

• Changelog entry added.
  • Explains the change in Changed, Fixed, Added sections.
  • For API change contains an example snippet and/or migration example.
  • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
  • FogBugz is marked as "Resolved" with next release version correctly set.
• Tests added/changed, if applicable.
  • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
  • Performance tests.
  • Integration tests.
• Docs for new/changed API's.
  • Xmldoc cross references are set correctly.
  • Added explanation how the API works.
  • Usage code examples added.
  • The manual is updated, if needed.

During merge:

• Commit message for squash-merge is prefixed with one of the list:
  • NEW: ___.
  • FIX: ___.
  • DOCS: ___.
  • CHANGE: ___.
  • RELEASE: 1.1.0-preview.3.

[vrdave-unity]

I think we missed the window on 1.6.2, but I'll talk to Lyndon about fast-tracking another patch release to fix this.

[chris-massie]

It was a bad merge, I fixed it by moving it from 1.6.2 to Unreleased.

[jamesmcgill]

Looks good.
You might need to pull in latest develop to fix up the Dry Run failures there (likely requires another CHANGELOG)

[jamesmcgill]

Would testing InputActionProperty.action for null rather than exposing these fields have the same result in TrackedPoseDriver?

It's implementation is:
public InputAction action => m_UseReference ? m_Reference != null ? m_Reference.action : null : m_Action;
which I think will return null in all cases m_Reference and m_Action will both be null.

Otherwise perhaps adding a new member here rather than exposing the private fields so that the TrackedPoseDriver code is not so dependent on how this is implemented.

[chris-massie]

It wouldn't be exactly the same. If a user clicked Use Reference to toggle the value in the Inspector window, the action property might start evaluating to null but one of its fields is already set. I wanted the migration code in TPD to skip copying the value from the old serialized field if any InputActionProperty value had already been created.

Instead of making the fields internal, I could expose all the serialized fields in new public properties, which goes along with the Unity coding style rules that all Inspector window values are accessible through API, but that would be an API addition and thus this PR couldn't be included in a patch release.

[jamesmcgill]

Ah right, I get the logic now.
There is another PR incoming that looks like it will need a minor bump, so now might be a good opportunity for getting something in (#1701).

m_Reference and m_Action have public properties already though and adding alternative versions of those that don't hold the invariants might be confusing. I don't know if exposing just m_UseReference will be enough for you to work with?

Alternatively perhaps another method like IsEmpty or IsNull to make the code in TrackedPoseDriver simple.

[chris-massie]

Even if I added a property for m_UseReference, I wouldn't be able to duplicate the logic exactly.

To try to address this feedback, I added new internal getter properties for the m_Action and m_Reference fields and reverted the fields back to private, and updated TPD to use those instead.

In the future, we can make them public and potentially add setters along with a property for m_UseReference, but we were scheduling to include this bug fix in a patch release so we can't add public properties yet.