Ocean Renderer OnEnabled lifecycle event

[daleeidd]

This will fix #576

For now I have only added the OnEnabled event. And I have only used that event for the planar reflections and underwater environmental lighting components.

If you think this is a good idea, we could take it further. Might even be able to use inheritance to hide most of it away Gave inheritance a go with the last commit. Can be reverted if not desired.

[huwb]

Thanks for this!

I generally would not change the meaning of OnEnable and OnDisable. I think its very important to maintain the semantic meaning of Unity's callbacks and not call it in other scenarios. This may lead to very confusing and evil behaviour where someone is wondering why OnEnable gets called an additional time (when the ocean woke up) and made a bunch of things happen.

I would vote for perhaps creating an OnOceanEnable() method and calling it from OnEnable(). That way the calling pattern of OnEnable does not change. Does something like this make sense?

[huwb]

Oh i just realised i'm not the biggest fan of wrapping MonoBehaviour. When another programmer sees MonoBehaviour they know exactly what to expect, whereas added a wrapper adds an additional layer they need to check. That other programmer could be me in a few months when i forget that we added this :) (or me in a few months to verify nothing else has changed in the wrapper..).

A reason i'm calling it out is I'm on a project right now with project wrappers around non-trivial things and its not a good time. This might be making me overly-sensitive to it though :), maybe this is fairly innoculous. Let me know what you think?

Is there a way to package up the logic that isn't in a base class perhaps?

[daleeidd]

Is there a way to package up the logic that isn't in a base class perhaps?

That's a fair point. I have dropped that commit. I can probably abstract it out to a helper class once we settle on a direction.

I would vote for perhaps creating an OnOceanEnable() method and calling it from OnEnable(). That way the calling pattern of OnEnable does not change. Does something like this make sense?

It does. So we are no longer going to disable the component on launch if the ocean renderer isn't preset?

[huwb]

Ok

Would it be possible to disable the component if no ocean, but still subscribe to ocean enable events, and wake up later if an ocean becomes avail?

[daleeidd]

It can. That is what it currently does. Only issue is that OnEnabled for these components are called twice because they are disabled from OnEnabled... I can move it to Awake so we never hit OnEnabled. But then Awake is called before OceanRenderer.Instance is set since it is set in OnEnabled. Not an issue though. It just means our event is always used. What do you think?

[huwb]

OnEnabled for these components are called twice because they are disabled from OnEnabled

Why would disabling call OnEnable?

I've reread the above but I haven't been able to understand. Maybe seeing the sequence of things that happen would help me visualise this, would that be possible?

[daleeidd]

Sure. We don't call OnEnable manually if that was the source of misunderstanding.

Frame 1
We enter play mode
Component.OnEnable is called by Unity as normal.
In Component.OnEnable we register for the OnOceanRendererEnabled event.
In Component.OnEnable it disables itself by setting enabled = false (Component.OnDisabled is called subsequently).
OceanRenderer.OnEnable is called (OceanRender executes later +200)
OnOceanRendererEnable event is invoked from OceanRenderer
Component.OnOceanRendererEnabled is called
Component is re-enabled by setting enabled = true

Frame 2
Component.OnEnable is executed because component was disabled but now enabled again.

The semantics are still intact since both OnEnable/OnDisable is called.

But then Awake is called before OceanRenderer.Instance is set since it is set in OnEnabled. Not an issue though. It just means our event is always used.

Can ignore this half-baked thought. It is the same for OnEnable since it always runs before the ocean renderer due to execution order. So every component that would use this event will disable itself when entering play mode since the ocean renderer would not be ready yet. Using Awake would be different to what has been done here and less robust I think.

[huwb]

Ok I think I'm understanding! That's quite a sequence

It's a shame that setting enabled to false and then true in the same frame triggers the OnDisable+OnEnable sequence, although understandable.

I thought of a couple of ideas and don't like either of them :/ so I'd propose to leave it as is - call the above sequence a feature and hope it doesn't bite us in the ass. As you say, semantics are intact.

[daleeidd]

It is unfortunate. If we do it in Awake it skips OnEnable and goes straight to OnDisable.

I was thinking of a possible comprehensive solution to both ocean enable and disable, and this is what it would look like:

using UnityEngine;
using Crest;

[ExecuteAlways]
public class DebugMonoBehaviour : MonoBehaviour
{
    // Store the user preference.
    bool _enabledPreference;
    // We need this to distinguish between user enabled and ocean renderer enabled. It is a possible race condition.
    bool _updateEnabledPreference;

    void Awake()
    {
        OceanRenderer.OnOceanRendererEnabled -= OnOceanRendererEnabled;
        OceanRenderer.OnOceanRendererEnabled += OnOceanRendererEnabled;
        OceanRenderer.OnOceanRendererDisabled -= OnOceanRendererDisabled;
        OceanRenderer.OnOceanRendererDisabled += OnOceanRendererDisabled;

        _enabledPreference = enabled;
        _updateEnabledPreference = true;
    }

    void OnEnable()
    {
        if (_updateEnabledPreference)
        {
            _enabledPreference = true;
        }
        else
        {
            _updateEnabledPreference = true;
        }

        if (OceanRenderer.Instance == null)
        {
            // False, because we do not want OnDisable changing the preference.
            _updateEnabledPreference = false;
#if UNITY_EDITOR
            if (Application.isPlaying)
#endif
            {
                enabled = false;
            }
            return;
        }
    }

    void OnDisable()
    {
        if (_updateEnabledPreference)
        {
            _enabledPreference = false;
        }
        else
        {
            _updateEnabledPreference = true;
        }
    }

    void Update()
    {
        // We still want this check for edit mode since we do not disable the component in edit mode to avoid
        // serialisation issues.
        if (OceanRenderer.Instance != null)
        {
            return;
        }
    }

    void OnDestroy()
    {
        OceanRenderer.OnOceanRendererEnabled -= OnOceanRendererEnabled;
        OceanRenderer.OnOceanRendererDisabled -= OnOceanRendererDisabled;
    }

    void OnOceanRendererEnabled()
    {
        _updateEnabledPreference = !_enabledPreference;
#if UNITY_EDITOR
        if (Application.isPlaying)
#endif
        {
            enabled = _enabledPreference;
        }
    }

    void OnOceanRendererDisabled()
    {
        _updateEnabledPreference = !_enabledPreference;
#if UNITY_EDITOR
        if (Application.isPlaying)
#endif
        {
            enabled = false;
        }
    }
}

It handles everything including ExecuteAlways. And always maintains the user's preference.

[daleeidd]

I have added the above as an abstraction. Handles both enabled and disabled. It also maintains the user's preference (whether by checkbox and scripting). Seems to be pretty solid. I haven't thought about possible race conditions since we rely on a little bit of state between enabled and disabled. But I think it should be ok.

I guess another alternative would be to have an isInitialized flag to manage initialisation. Then we would check this flag in Update (or whichever runs every frame) along with OceanRender.Instance.

[daleeidd]

@huwb I think we should revisit this one. If you think this is too heavy then the alternative is as above:

...to have an _isInitialized flag to manage initialisation. Then we would check this flag in Update (or whichever runs every frame) along with OceanRenderer.Instance.

In addition, the alternative would need to change it so it never disables the component in edit mode which we currently do. A good example is OceanDepthCache disables itself in prefabs since OceanRenderer.Instance is not available.

One downside of the alternative is that we log errors on validation failure which means we would spam the logs unless we also have a check for that too.

[huwb]

Thanks i think something like this can work

I found its quite dense logically and was not able to understand the aspects of it. i probably could if i traced it all out and took notes but i thought it was maybe a good opportunity to embrace the confusion and ask for more docs (and maybe var renames if any come to mind)!

[huwb]

could you add more notes about what it does and why (info from the PR discussion).

[daleeidd]

Sure thing. Done.

[huwb]

maybe another comment - i think i know what the bool represents, but i cant quite get my head around how it works / how it has the right value at this time

[daleeidd]

I added a comment. We set it to false whenever we know it won't be the user.

[daleeidd]

Done. Turns out it was not robust as I thought. I went through it again and got it working correctly. It should look a little simpler too. Components executing in edit mode still have to have some sort of initialised flag unfortunately since this component doesn't manage edit mode to avoid changing serialised data.

[klauskobald]

I am picking this up without reading the whole story. Start, Awake, Enable and Disable is often giving me problems. In special because OnEnable is also called before Start. What I do now in my projects is using events and listeners.
You should have something like this in the OceanRenderer class

 public enum Message
    {
      WillInitialize,
      WillBuildLods,
      HasBuildLods,
..........
      Initialized,
      Ready
   }

    public delegate void Notification(Message msg);
    public static event Notification onNotification;

emit events by calling onNotification(Message.WillInitialize);

so everybody could listen to theses events by OceanRenderer.onNotification+=MyCallback;
You can create a chain of asynchronous calls and also "we" could listen to these events and act accordingly.

All the components could also have their events. But mabye not over-engineer it. You could also make one centralized independent static class that handles all crest events, which may be better.

[huwb]

This change adds the kind of functionality you mention Klaus, but also deals with various combinations of ocean system being present/non present/enabled/disabled at various times relative to other components.

It's a thumbs up from me.

[daleeidd]

Turns out a case this doesn't cover very well:

_target.enabled = true;
_ocean.enabled = false;
Assert.IsFalse(_target.enabled); // passing
// This case was fine using the editor toggle, but not here. We have to _target.enabled = true; before so it can store the state.
_target.enabled = false;
_ocean.enabled = true;
Assert.IsFalse(_target.enabled); // failing

This worked fine in the editor since it triggers an OnEnable which we handle and store the user preference as false. But in a script it won't trigger anything. Could be considered and edge case. Or we could have a proxy "enabled" for scripting, but that won't be seamless to the user either.

Alternatives is to not use FixedUpdate or Update and have components use OnOceanRendererUpdate instead.

Or just not disable components and let users handle the performance savings themselves. If we want delayed initialisation, we could have a flag and initialise in FixedUpdate/Update if initialisation failed due to no ocean being present.

[huwb]

ah ok. so the target doesnt receive an event when _ocean.enabled = true ? why doesnt this work outside of editor? just curious

[huwb]

per frame update would ideally be avoided.

if this is not going to work as is maybe we should bring out the big guns - make an interface IInterestedInOceanState and give it OnOceanEnabled method, then when ocean renderer is enabled:

var interestedComps = FindObjectsByType<IInterestedInOceanState>();
foreach( var comp in interestedComps ) OnOceanEnabled();

not tested but i think it should work. but may cause a small hitch.

maybe a less hitchy solution is to have a static list of interested components on the OceanRenderer, and each interested thing registers itself with that list, and then the ocean renderer iterates over the list and notifies things.

[klauskobald]

This should probably change to
foreach( var comp in interestedComps ) comp.OnOceanEnabled();
Another thought:
If you do it like this any components that are instanciated after the ocean is created will no get the message. So you should maybe also add something like OceanRenderer.RequireStateMessage(), which a new component could call to inform the oceanrenderer, that it needs to inform everybody again. There should be a _requireTrigger property and a coroutine/invoke that handles this async, so that the trigger is only called once with a tiny timeout (200ms) after all requests have been made.

[daleeidd]

ah ok. so the target doesnt receive an event when _ocean.enabled = true ?

The issue is different. If we set the component to disabled then the user cannot with scripting. Above test case in plain english:

• Component is enabled
• Ocean becomes disabled
• Component becomes disabled automatically because ocean becomes disabled
• User wants to set component to disabled for some other reason
• Component is already disabled so enabled = false will not do anything but we cannot record their value either
• Ocean becomes enabled
• Component becomes enabled which is not what the user wants

why doesnt this work outside of editor?

It's sort of there too. The difference in the editor UI is that we can inform the user that the component is disabled because of the ocean renderer. If they click the toggle when it is false it tried to enabled the component which allows us to capture the value. For this to work in scripting, the user would have to enabled = true which is a little confusing. It's something I should have thought about early on but didn't :(

maybe a less hitchy solution is to have a static list of interested components on the OceanRenderer, and each interested thing registers itself with that list, and then the ocean renderer iterates over the list and notifies things.

Sound good. This would have to include:

• OnOceanEnabled
• OnOceanDisabled
• OnOceanUpdate
• OnOceanLateUpdate
• OnOceanFixedUpdate

We could even pass the ocean instance as a parameter and reduce dependency on the singleton, and use C# delegates which is more accessible as an API. Let me know what you think.

If you do it like this any components that are instanciated after the ocean is created will no get the message. So you should maybe also add something like OceanRenderer.RequireStateMessage(), which a new component could call to inform the oceanrenderer, that it needs to inform everybody again. There should be a _requireTrigger property and a coroutine/invoke that handles this async, so that the trigger is only called once with a tiny timeout (200ms) after all requests have been made.

There are a few ways to handle that case which sounds like ocean dependent initialisation. A flag on the interested component (eg isInitialized) and if false (checked in OnOceanUpdate) then calls initialisation function just in case it missed OnOceanEnabled. Components can handle it themselves easily so I don't think it's necessary unless I missed something?

[huwb]

sorry i didnt manage to get back to this

We could even pass the ocean instance as a parameter and reduce dependency on the singleton, and use C# delegates which is more accessible as an API. Let me know what you think

yeah i think this is worth trying