KybernetikGames / animancer

Documentation for the Animancer Unity Plugin.
66 stars 8 forks source link

Should Transition Assets use [SerializeReference]? #132

Closed KybernetikGames closed 3 years ago

KybernetikGames commented 3 years ago

Use Case

ClipTransitionSequence is a regular serializable class so it can be configured directly in the Inspector of your script instead of needing to create a separate Transition Asset to hold it, but that also means it would need another Transition Asset class to be declared if you do want to use it that way.

The same applies to any other custom transition you make. If you want to use it as a Transition Asset, you need to declare an extra class for it (and an UnShared class if you want that too).

Solution

The next version of Animancer will support the [SerializeReference] system introduced in Unity 2019 which allows for inheritance in serialized fields.

If we apply that attribute to Transition Assets then a ClipTransition asset that would previously only be able to have a ClipState.Transition will now let you select any other transition type that inherits from it:

transition-asset-serialize-reference

Benefits

Problems

I'm leaning towards implementing this change since I prefer to see forward progress rather than being tied down by old API limitations, but I'm open to suggestions.

tatoforever commented 3 years ago

Hi, I'm also would like this to be implemented but I have tons of references through different Transition Assets. What about an upgrade script that temp saves old references for each transition as an scriptable object then we can proceed to install new version and the upgrade script will setup references again? You can make it a MenuItem with two options:

KybernetikGames commented 3 years ago

That gives me a few ideas for potential ways to keep the data:

1. Manual Upgrade Script

  1. You download an upgrade script that I'll put in the Change Log's Upgrade Guide.
  2. When imported, it finds all Transition Assets, creates a ScriptableObject in which it stores references to each of them and copies of their data.
  3. You import the new version of Animancer.
  4. When the upgrade script detects the new version, it will distribute all the gathered data back to the corresponding Transition Assets then delete itself.

Pros

Cons

2. Dummy Serialized Field

  1. I give the base Transition Asset class these fields:
#if UNITY_EDITOR
[SerializeField]
[HideInInspector]
[UnityEngine.Serialization.FormerlySerializedAs("_Transition")]
private TTransition _Animation;
#endif

[SerializeReference]
private TTransition _Transition;
  1. The [FormerlySerializedAs] attribute will let the _Animation field will pick up the old data from when _Transition was a regular [SerializeField].
  2. The ReadMe already has a system in place to determine if you're importing into a project that had an older version of Animancer. So when it detects that, it can find all Transition Assets and copy the contents of their _Animation field onto their _Transition field (which is the one that actually gets used) so they are all using the new system.

Pros

Cons

3. Fiddle with the assets as text

  1. When the ReadMe detects an upgrade from an older version of Animancer, find all Transition Assets, read them as text, modify their _Transition field to the format required for [SerializeReference], and re-save them.

The format is relatively simple. The old one looks like this:

  _Transition:
    _FadeDuration: 0.25
    // ... Other fields in the transition.

And the new one looks like this (comments mine):

  _Transition:// The actual field
    id: 0// The index of the reference
  // Other fields in the Transition Asset class would go here, each simply referencing an index in the final list of references where the data is.
  references:// The list of all references
    version: 1
    00000000:// The id: 0 refers to this reference
      type: {class: LinearMixerState/Transition, ns: Animancer, asm: Animancer}// The assigned type
      data:// The actual data all looks the same
        _FadeDuration: 0.25
        // ... Other fields in the transition.

Pros

Cons

KybernetikGames commented 3 years ago

4. Procedural Dummy Serialized Field

A variant of # 2.

Instead of adding the dummy field directly, when the ReadMe detects the version upgrade it loads the Transition Asset script as text and adds in the dummy field, then does its thing, then modifies the script again to remove the dummy field.

Pros

Cons

KybernetikGames commented 3 years ago

5. Conditional Dummy Serialized Field

Another variant of # 2, but instead of editing the script we put the dummy field in a #if ANIMANCER_UPGRADE block so the ReadMe can add that symbol to the project settings, do the upgrade, and then remove it.

Pros

Cons

#if UNITY_2019_1_OR_NEWER
#if UNITY_EDITOR
#pragma warning disable CS0649 // Field is never assigned to, and will always have its default value.
[SerializeField]
[HideInInspector]
[UnityEngine.Serialization.FormerlySerializedAs("_Transition")]
private TTransition _Animation;
#pragma warning restore CS0649 // Field is never assigned to, and will always have its default value.
#endif

[SerializeReference]
#else
[SerializeField]
[UnityEngine.Serialization.FormerlySerializedAs("_Animation")]
#endif
private TTransition _Transition;
tatoforever commented 3 years ago

I would go with one or two. But if I have to chose between the two it would be 1, you just need to make sure references serialization will be done first by doing some pre-processing editor script compilation, so when editor finish compiling the new version then you can fire some script post processing to set new references. Or simply by having the user save all old references with a MenuItem tool in a script you provide then proceed to the upgrade. I'm not a fan of keeping hidden legacy stuff. Cheers,

KybernetikGames commented 3 years ago

TL;DR: it looks like I'll have to move and rename all transition classes.

I've started writing an upgrade script for Approach # 1 and it turns out there's another problem that [SerializeReference] is going to cause. Regular serialized fields don't serialize their actual type because the type always matches the field, but references do need to serialize the name of their type and the assembly it's in, which causes problems when upgrading from Animancer Lite to Pro because Lite has all the transition classes defined in the Animancer.Lite assembly but Pro compiles them into the Animancer assembly with all the other scripts.

That means upgrading from Lite to Pro at any time in the future (not just for this Animancer version upgrade) would cause you to lose any transition data stored in [SerializeReference] fields. That isn't acceptable since the upgrade process needs to be seamless. I want people to be able to start working in Lite then decide to upgrade to Pro without losing any of their work.

This could be worked around with a process that edits assets as text like Approach # 3, but that would never be reliable enough.

A much better solution would be to move all the transition classes out to the Transition Assets scripts (Assets/Plugins/Animancer/Utilities/Transitions) since the Utilities folder is included as regular scripts in Animancer Lite rather than compiled into the DLL so the assembly wouldn't change when upgrading to Pro.

So what's the problem?

If the transition classes aren't in the same assembly as the state classes, they can no longer be declared as nested classes, i.e. they can't be ClipState.Transition which means we need a new naming convention for them.

My current preference for the new convention is:

Old Name New Name
ClipState ClipState (no change)
ClipState.Transition ClipTransition
ClipTransition ClipTransitionAsset (I would have gone for ClipTransition.Asset, but it's a ScriptableObject class so Unity won't allow that because the name has to match the file name)

But that has the notable drawback of ClipTransition currently being a ScriptableObject reference but turning into a regular serializable class, which could be confusing to anyone familiar with the system.

It also means that upgrading from an earlier version of Animancer will cause compile errors as you need to go through all your scripts to change to the new names. Though that could be done using an automated text editing process since it's entirely script editing (with a confirmation dialogue box telling users to backup their project first like Unity's upgrader).

Thoughts? Ideas for a better naming convention?

tatoforever commented 3 years ago

I know this is not totally what you are trying to do but this might come handy when refactoring MonoBehaviors ScriptableObjects: https://docs.unity3d.com/ScriptReference/Serialization.FormerlySerializedAsAttribute.html

KybernetikGames commented 3 years ago

I've used that attribute in a few places, but unfortunately it only works for changing field names, not for changing from a [SerializeField] to a [SerializeReference] or for the assembly issue.

tatoforever commented 3 years ago

Kinda tricky yeah.

KybernetikGames commented 3 years ago

Animancer v7.0 is now available with these changes in it.