[BUG] Recompiling during play mode resets all variables

[lgarczyn]

Describe the bug

Whenever I change any script while the game is running, most systems involving atoms break down, while others are unaffected.

After looking into it, it appears all vars reset their value (without emitting any event).

To Reproduce

Steps to reproduce the behavior:

1. Create a bool var with default value false
2. Enter play mode
3. Set value of bool var to true
4. Modify a script
5. After recompilation, bool var is now false

Expected behavior

Live recompilation's purpose is the modify the code while preserving the state as best as possible. It's virtually unusable on any project using atoms.

Environment

• OS: Windows
• Unity Version: 2022.3.11f1
• Version of Unity Atoms: 4.4.8

[soraphis]

what are your settings about fast enter playmode?

[lgarczyn]

what are your settings about fast enter playmode?

Reload domain and reload scene set to false

[lgarczyn]

I might have a found the source for some of the reset, in AtomBaseVariableInstancer.cs there's this extremely weird bit:

        private void OnEnable()
        {
            if (Base == null)
            {
                _inMemoryCopy = ScriptableObject.CreateInstance<V>();
            }
            else if(_inMemoryCopy == null)
            {
                _inMemoryCopy = Instantiate(Base);
            }
            ImplSpecificSetup();
        }

So if the instancer has no base var, it's always reset on OnEnable
(and too bad for anyone listening to the old instance)
but if it DOES have a base var, then it checks that it actually needs a reset?

Why would a base var change the behavior so much?

[lgarczyn]

The others are simply called through OnEnable

I don't think any part of the code takes in account that OnEnable may be called multiple times on a SO.

Why don't Variables init on Awake instead ?

        protected virtual void Awake()
        {
            SetInitialValues();
        }

        protected virtual void OnEnable()
        {
            TriggerInitialEvents();

#if UNITY_EDITOR
            if (EditorSettings.enterPlayModeOptionsEnabled)
            {
                _instances.Add(this);

                EditorApplication.playModeStateChanged -= HandlePlayModeStateChange;
                EditorApplication.playModeStateChanged += HandlePlayModeStateChange;
            }
#endif
        }

        private void OnDestroy()
        {
            // NOTE: This will not be called when deleting the Atom from the editor.
            // Therefore, there might still be null instances, but even though not ideal,
            // it should not cause any problems.
            // More info: https://issuetracker.unity3d.com/issues/ondisable-and-ondestroy-methods-are-not-called-when-a-scriptableobject-is-deleted-manually-in-project-window 
#if UNITY_EDITOR
            _instances.Remove(this);
#endif
        }

[lgarczyn]

I've pushed my version that tries to avoid these problems here:

https://github.com/lgarczyn/unity-atoms/tree/master

[soraphis]

I don't think Awake is the correct approach here.

first, Awake seems to be somewhat broken... It Also seems to be more like a "OnCreated" callback, which is NOT the moment to call "SetInitialValues" (as it wouldn't do anything then).

We might not need the call at all - but I'm not certain about that - as the playModeStateChange events might be good enough.

There are ultimatively a couple of different cases to check:

IntVariable

• must be set to it's initialValue when leaving playmode
• must Trigger InitialEvents when entering playmode
• must keep it's value when unity hot reloads
• should not trigger Initial events when hot reloading

Furthermore:

• Variables in an IntVariableInstancer must behave exactly the same.
• All tests need to be run (and pass) with and without Domain Reload.

sadly only 2(×2) of those can be checked with unit tests. As we can't really change the domain reload thing with unit tests and can't rly check the hot reload with unit tests ...

I feel there is still a lot more to be checked and taken into account, but I don't remember exactly all the issues we had with this in the past...

[lgarczyn]

You should be able to check domain reload through unit tests by using this:

https://docs.unity.cn/Packages/[email protected]/api/UnityEngine.TestTools.WaitForDomainReload.html

And calling https://docs.unity3d.com/ScriptReference/EditorUtility.RequestScriptReload.html

SetInitialValues is required to be called on Awake or OnEnable to ensure that the serialized current and previous value are not mismatched with the initial value, which maybe could happen ?

It's also required to ensure that the callback for EnterPlayMode is registered, but that could be handled using many other systems.

[lgarczyn]

I also fixed the instancer bug: lgarczyn@267eaeb

[soraphis]

SetInitialValues is required to be called on Awake or OnEnable to ensure that the serialized current and previous value are not mismatched with the initial value, which maybe could happen ?

SetInitialValues in Awake has no effect, since it is only called at asset creation, _value, _oldValue and InitialValue will all be default.
It is required in OnEnable, to make sure that the (hidden) _value field is correctly InitialValue for the start of the game. Also _oldValue has to be the same here because for the initial events that are triggered it make no sense to have _oldValue at a value of the previous playmode session.

while HandlePlayModeStateChange will also call instance.SetInitialValues() it might not be necessary to have that call in OnEnable ... #if UNITY_EDITOR but within a build, I think it has to be in OnEnable ...

It's also required to ensure that the callback for EnterPlayMode is registered, but that could be handled using many other systems

If it is called in Awake, it will not register the EnterPlayMode callbacks frequent enough, that part HAS to be in OnEnable

I also fixed the instancer bug: lgarczyn@267eaeb

🤔 I guess this is okay. At least i can't think of an case where this would break something.

(but for real, imho the Instancers require a rewrite (#370)

[lgarczyn]

SetInitialValues in Awake has no effect, since it is only called at asset creation, _value, _oldValue and InitialValue will all be default.
It is required in OnEnable, to make sure that the (hidden) _value field is correctly InitialValue for the start of the game. Also _oldValue has to be the same here because for the initial events that are triggered it make no sense to have _oldValue at a value of the previous playmode session.

I can think of two effects:

• Copy-pasting an atom
• Deserializing an atom from JSON

In both those case, awake would not be called with the default value.