Closed hadashiA closed 6 months ago
vercel[bot]
commented
6 months ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| vcontainer | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Mar 31, 2024 3:04am |
hekk-naoya-kishimoto
commented
6 months ago Thank you for your prompt response!
Verification of the Issue (https://github.com/hadashiA/VContainer/issues/652) was not sufficient. We have since found the following and would like to propose a fix as well.
As for the behavior of ScriptableObject in UnityEditor,
After launching the UnityEditor, if you select a ScriptableObject (VContainerSettings) or an object that has VContainerSettings serialized to it (including PlayerSettings with the PreloadedAssets), the ScriptableObject is loaded and Awake() => OnEnable() executed.
If Awake() is executed even once, when recompiling or pressing the play button, Awake() is not executed and OnDisable() => OnEnable() seems to be executed in that order.
If Awake() is never been executed, neither Awake() nor OnEnable() nor OnDisable() seems to be executed when recompiling or pressing the play button.
(UnityEditor internal behavior is to cache them as ScriptableSingleton...?)
So, if we believe the above behavior, the following implementation seems to be a good choice.
How about using [InitializeOnLoadMethod] instead of [RuntimeInitializeOnLoadMethod] for UnityEditor?
If you use [InitializeOnLoadMethod], it should basically always be executed when UnityEditor starts up, considering the behavior of ScriptableObject, I think it would be better to Awake() the ScriptableObject systematically once, so that it is not affected by the user's operation status, and the behavior can be more consistent.
By doing Awake() once, it will ride on UnityEditor's lifecycle from then on, so there is no need to explicitly instance.OnEnable().
#if UNITY_EDITOR
[UnityEditor.MenuItem("Assets/Create/VContainer/VContainer Settings")]
public static void CreateAsset()
{
~~~
}
[InitializeOnLoadMethod]
static void InitializeOnLoad()
{
var instance = UnityEditor.PlayerSettings.GetPreloadedAssets().FirstOrDefault(x => x is VContainerSettings);
if (instance == null)
{
Debug.LogError($"VContainerSettings is not registered in PreloadedAssets.");
}
}
#endifAt this time, as a tutorial guidance to the user, it would be good to consider situations where VContainerSettings does not exist or is not registered in PreloadedAssets, and to issue an error or something similar.
Also, [RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] is executed later than PreloadedAssets and [InitializeOnLoadMethod], OnDisable() => [InitializeOnLoadMethod] => OnEnable() => [RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType. BeforeSceneLoad)] in that order.
If I load VContainerSettings with [RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] or something,
I don't think this is a good idea because there is a subtle difference in timing between UnityEditor and Runtime (device).
I'm aware that OnEnable() is only executed once, so if (Instance == null) seems like it could go either way, I would like it to work singleton-like, including subsequent processing, so I think it would be a good idea to check null to let it win first, in case it runs in duplicate.
Also, in relation to eliminating if (Application.isPlaying), OnEnable() is executed even when the play button is not pressed, and OnFirstSceneLoaded() => GetOrCreateRootLifetimeScopeInstance() is executed.
In this case, an Exception is raised because it cannot DontDestroyOnLoad.
Furthermore, until now, UnityEditor has been loaded with [RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)], I think the Runtime (device) had OnFirstSceneLoaded consideration due to the fact that it was loaded with PreloadedAssets.
With this fix, both UnityEditor and the runtime (actual device) will be loaded with PreloadedAssets, so it will always be before the scene is loaded, but UnityEditor's behavior is such that even in situations where the playback button is not pressed or immediately after pressing the playback button SceneManager.GetActiveScene() will take the scene and activeScene.isLoaded will be true.
In this case, as above, an Exception is raised because DontDestroyOnLoad is not possible.
How about the following modifications, including a solution to this problem?
void OnEnable()
{
#if UNITY_EDITOR
if (!EditorApplication.isPlayingOrWillChangePlaymode)
{
return;
}
#endif
if (Instance == null)
{
Instance = this;
SceneManager.sceneLoaded -= OnFirstSceneLoaded;
SceneManager.sceneLoaded += OnFirstSceneLoaded;
}
}I think it is better to have it because OnDisable() does not leave a cache of static Instance and the behavior should be the same every time.
void OnDisable()
{
Instance = null;
}Best regards.
(ちょっと英語だと自信ないので、日本語も併記しておきます！)
早速ご対応いただきありがとうございます！
Issue (https://github.com/hadashiA/VContainer/issues/652) の検証では不十分で、その後以下のようなことがわかったため、併せて修正案もご提案させていただきます。
UnityEditor の ScriptableObject の挙動として、
UnityEditor を起動後、対象の ScriptableObject (VContainerSettings) を選択したり、VContainerSettings がシリアライズされているオブジェクト(PreloadedAssets の設定がある PlayerSettings も含む) を選択したりすると、対象の ScriptableObject がロードされて、Awake() が実行されたあと、OnEnable() が実行されるようです。
1度でも Awake() が実行された場合は、リコンパイルしたり再生ボタンを押したときに、Awake() は実行されず、OnDisable() => OnEnable() の順で実行されるようです。
1度も Awake() が実行されていない場合は、リコンパイルしたり再生ボタンを押しても、Awake() も OnEnable() も OnDisable() も実行されないようです。
(UnityEditor の内部挙動として、ScriptableSingleton としてキャッシュされている…？)
なので、上記挙動を信じた場合、以下のような実装が良いように思います。
UnityEditor の場合、[RuntimeInitializeOnLoadMethod] ではなく [InitializeOnLoadMethod] を使うのはどうでしょうか？
[InitializeOnLoadMethod] を使用した場合、基本的に UnityEditor の起動時に必ず実行されるはずで、ScriptableObject の挙動を考慮すると、システム的に必ず1度 Awake() しておくことで、ユーザーの操作状況などにされず、より挙動を統一できるかと思います。
1度でも Awake() しておくことで、以降は UnityEditor のライフサイクルに乗るため、明示的に instance.OnEnable() などは不要に思います。
このとき、ユーザーへのチュートリアル的な誘導として、VContainerSettings が存在しなかったり、PreloadedAssets に登録されていない状況を考慮して、エラーなどを出しても良いと思います。
また、[RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] は、PreloadedAssets や [InitializeOnLoadMethod] より遅れて実行され、再生ボタンを押した後、OnDisable() => [InitializeOnLoadMethod] => OnEnable() => [RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] の順で実行されます。
[RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] で VContainerSettings をロードしたりすると、UnityEditor とランタイム(実機)で微妙なタイミングの違いがあるため、あまり良くないように思います。
OnEnable() は、1度しか実行されない認識なので、if (Instance == null) は、どちらでも良さそうな気がしますが、
後続の処理含めてシングルトン的に動作してほしいと思うので、もし二重で動いた場合を考慮して、先勝ちさせるため、null チェックしておくと良さそうに思います。
また、if (Application.isPlaying) を削除する関係で、
再生ボタンを押していないときでも、OnEnable() が実行され、OnFirstSceneLoaded() => GetOrCreateRootLifetimeScopeInstance() と実行されてしまいます。
このとき、DontDestroyOnLoad できないため、Exception が発生します。
さらに、今まで UnityEditor の場合は、[RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] でロードされていて、ランタイム(実機)は、PreloadedAssets でロードされていた関係で、OnFirstSceneLoaded() の考慮があったと思います。
今回の修正で、UnityEditor もランタイム(実機)も、PreloadedAssets でロードされるようになるため、必ずシーンのロード前になりますが、UnityEditor の挙動として、再生ボタンを押していない状況や再生ボタンを押した直後でも SceneManager.GetActiveScene() でシーンが取れてしまい、activeScene.isLoaded は true となります。
このときも上記と同様で、DontDestroyOnLoad できないため、Exception が発生します。
この問題の対策を含めて、以下のような修正はどうでしょうか？
OnDisable() で static な Instance のクリアすることで、キャッシュが残らず、毎回同じ挙動になるはずなので、あった方が良いと思います。
引き続き宜しくお願い致します。
hadashiA
commented
6 months ago @hekk-naoya-kishimoto こんにちは。 この辺りの挙動をここまで詳細に追っていなかったので、大変参考になりました。ありがとうございます!
以下、いただいた提案について考えてみました。
RuntimeInitialize() の修正案
自動的なOnEnableに頼れるならたしかにそのほうがよさそうです。
ただ、以下の点で懸念がありました。
Unity再起動、リフレッシュ、PlayMode on/off などの操作と、Domain Reloadingの設定などの組み合わせのパターンすべてが動く必要があるため、実行時に設定されているものを明示的に有効にするほうが結局問題をコントロールしやすい面はあると思います。 そのため、ここについては 以前の方針のままでいこうと考えてます。
OnEnable() の修正案
たしかにエディタでエラーが出てしまいますね。playmodeのチェックは結局必要そうです。
ただ、手元だと EditorApplication.isPlayingOrWillChangePlaymode = true かつ isPlayng = false の場合、DontDestroyOnLoad に失敗してしまいます。
ここは isPlaying でよさそうに見えました。
また、Domain Reloading 無効の場合を考えると、Instance == null でない場合も上書きできないとまずい場合があるかもしれません。
といったことを踏まえて、結局、1.14.0時点のものまで一度戻し、二度初期化されているバグだけ対応することにしました。
現在のリリース版では、Editor上でRootLifetimeScopeがロードされなくなった、という問題があるようなので、取り急ぎ一度リリースはしてしまおうと思っています。
この辺に改善点があることは確かなので、また何かあればお願いします 🙏
648 #652