Search Unity

MonoBehaviour.OnDisable() should only be executed ONLY when disabled. NOT ALSO when destroyed

Discussion in 'Editor & General Support' started by reddtoric, Aug 12, 2019.

?

Thoughts on this issue

  1. Agree, OnDisable should ONLY be called when object is disabled and not also when destroyed

    39.6%
  2. Disagree, stay the same

    60.4%
  1. reddtoric

    reddtoric

    Joined:
    Mar 7, 2017
    Posts:
    7
    What if we only want code to only execute on disabled but it isn't destroyed?! Why is OnDisable function called by 2 causes when it should only be when object is on disabled? Unity has an OnDestroy function.

    OnDisable doc says "This is also called when the object is destroyed and can be used for any cleanup code. "

    https://docs.unity3d.com/ScriptReference/MonoBehaviour.OnDisable.html
    https://docs.unity3d.com/ScriptReference/MonoBehaviour.OnDestroy.html


    For me, this issue caused a missing reference exception because OnDisable is also called when object is destroy. I have code that needs to be executed only when on disable but not on destroy and because of this, I get undesirable results.

    Temporary solutions: https://answers.unity.com/questions/882428/ondisable-getting-called-from-destroy.html
     
    Last edited: Aug 12, 2019
    AntonPetrov and aparajithsairam like this.
  2. karl_jones

    karl_jones

    Unity Technologies

    Joined:
    May 5, 2015
    Posts:
    8,282
    I would have a rethink about how you are doing things. OnDisable has been like that for a long time, Imagine how many projects would break if we changed it now.

    What exactly is your code doing?
     
  3. Vryken

    Vryken

    Joined:
    Jan 23, 2018
    Posts:
    2,106
    Disabling an object should not cause a
    MissingReferenceException
    , because the object still exists.
    Destroying and object can, however, which means unless you're explicitly destroying the object inside of
    OnDisable()
    , whatever issue you're getting is not related to
    OnDisable()
    .

    Simply put: a
    MissingReferenceException
    is thrown when you try to reference an object that has been destroyed in the scene.

    I have a feeling that the issue here is with the way you've structured your code. You should really revise it (or post it here for help) before jumping to the conclusion that the issue is with Unity Engine itself.
     
    Last edited: Aug 12, 2019
    Joe-Censored likes this.
  4. reddtoric

    reddtoric

    Joined:
    Mar 7, 2017
    Posts:
    7
    I've already fixed my issue with a simple solution which doesn't use OnDisable(). But there are other people that need an OnDisable() that only executes when the object is disabled.

    Many projects would break. But saying that OnDisable() has been like that for a long time doesn't mean it should continue to be that way. The issue is why isn't there a function that is called only when the object is disabled?? The function is called OnDisable not OnDisableAndOnDestroy().

    You misunderstood me. Disabling doesn't caused the MissingReferenceException, destroying objects from unloading the scene caused it.
     
    a436t4ataf likes this.
  5. Billy4184

    Billy4184

    Joined:
    Jul 7, 2014
    Posts:
    6,022
    I agree with OP, this really tangles a few fundamentally different events.

    I have code that executes functions across multiple components from OnDisable (intended to be run only when the object is explicitly and individually disabled) and OnDestroy (intended to be run only when the object is explicitly and individually destroyed).

    However in the case of a scene change, those other components have already been destroyed (and would never otherwise be destroyed).

    So unless I am mistaken, I am forced to operate all the time as if any or all dependencies might have already been wiped out, when this only happens in a specific scenario (scene is unloaded) which I somehow am not able to detect beforehand.

    I believe the problem is clear enough to see without having to see one person's specific code. A single object being destroyed in the course of the game, and an entire scene being unloaded should be differentiated as a matter of course, in my opinion.
     
    a436t4ataf likes this.
  6. Discipol

    Discipol

    Joined:
    May 6, 2015
    Posts:
    83
    If the gameobject is inactive, neither of them is called. I can't trigger a pseudo "destructor" in my components because neither of the methods are called. How do I detect the component destruction regarldess or gameobject or component enabled state?
     
  7. MUGIK

    MUGIK

    Joined:
    Jul 2, 2015
    Posts:
    481
    That is a stupid situation:)

    I want:
    - in OnEnable allocate some data, send some signals to start component to work
    - in OnDisable clear data, send signals to stop component work, which requires some additional allocations on this game object.
    - to be able to enable/disable component during runtime

    Everything is OK until I want to Destroy an object with this component.
    Destroy is already an expensive operation, but it also calls OnDisable, which in my case, again, requires some additional allocations in another component on the same game object.

    The problem is that I don't need this allocation when game object is going to destroy, because, obviously it will be destroyed and this information won't be used.

    It would be easy peasy if we would have some flag that says that game object is going to destroy.
    I could check for this flag and don't make pointless allocations. But as I know there is no way to know that game object is going to be destroyed in OnDisable.
    Does this make sense?


    Yes, I could create my own methods like this:
    Code (CSharp):
    1. public void CustomEnable() {}
    2. public void CustomDisable() {}
    But then I won't be able to disable component from the inspector, which is standard way of doing things in Unity
    upload_2021-1-19_12-48-9.png
     
  8. reddtoric

    reddtoric

    Joined:
    Mar 7, 2017
    Posts:
    7
    Single responsibility principle and naming methods with meaningful names thrown right out the window.

    > OnDisable has been like that for a long time, Imagine how many projects would break if we changed it now.

    Legacy API's get depreciated. New methods are offered. Does keeping `OnDisable()` exactly as it is imply a new method (now with a less meaningful name) can't be offered for only when disabled?
     
    Last edited: Mar 22, 2021
  9. Qriva

    Qriva

    Joined:
    Jun 30, 2019
    Posts:
    1,314
    I encountered problem related to this again and it is really annoying, I don't get how this broken feature can ignored.
    I 100% agree with "Imagine how many projects would break", but I must criticize lack of support or will to fix the problem.

    First of all, yes, documentation mentions calling OnDisable when destroyed, but there is not a single word about consequences. I am pretty sure, that it's almost impossible to predict the problem at first glance (explained why below) and we are all here, because we experienced some problems first.
    Second, OnDisable from destruction (seems) do not follow any execution order. There is no guarantee which object will exist. The consequence are random ghost bugs, that are very hard to debug, becuase your code can work perfectly fine 30 times, and then crash in 31th and not appear again for another 20 tries (or vice versa). This is especially hard to find when application quits.

    It is hard to reliably detect when OnDisable is called during destruction and there are is no alternative event.
    Maybe this is breaking change, but why OnDisable can't be called before destruction of all objects like Start() does when application starts? Maybe additional event similar to OnDsiable could do the job, and If there is really no way to fix the problem, can you put any effort to prevent it somehow by providing example in documentation?
     
  10. Joe-Censored

    Joe-Censored

    Joined:
    Mar 26, 2013
    Posts:
    11,847
    I suspect the issue is, in your case, how OnDisable fits in with the destruction process as multiple objects are destroyed. What appears to be the expected behavior is Unity goes one by one through each object in the scene, in whatever order it does so, and calls OnDisable, followed by OnDestroy, and then actually destroys the object. Then it moves on to the next object. So since Unity itself controls the order in which the objects are destroyed, there's no guarantee that any other object still exists during OnDisable.

    I'd like to see a system where a callback is called before all of this, when everything is guaranteed to not be destroyed yet. Maybe call it OnAboutToDestroy or something. Sort of like how when you load a scene you know all Awake methods will be called for all objects, followed by all Start methods.

    I'm not sure what to suggest other than ample null reference checks in your OnDisable methods.

    Some more info on OnDisable/OnDestroy order:
    https://forum.unity.com/threads/is-ondisable-ondestroy-execution-order-done-per-object.540140/
     
    NotaNaN and Billy4184 like this.
  11. Qriva

    Qriva

    Joined:
    Jun 30, 2019
    Posts:
    1,314
    This is exactly what I deduced from this behaviour and this is why I proposed to call all OnDisable before any destruction. I don't think it would break something, it just guarantee order of execution when scene is unloaded, but only devs know if that's the case.

    The problem is, it's not just destroy intruction, adding and removing c# events can lead to situation where event is rised to already destroyed component where 'this' and owning game object does not exist anymore. If there is no other way than null checks everywhere then fine, I will do that, but I am sad, because me and probably many, many other people got tricked by this nasty design and there was no one to at least put sign "AHTUNG! Landmines ahead".
     
    Joe-Censored likes this.
  12. Billy4184

    Billy4184

    Joined:
    Jul 7, 2014
    Posts:
    6,022
    Exactly. I think it's clear enough that mixing the context of a single object being disabled and the destruction of the entire scene (not just that, but entering the context in the middle of it!) is an architectural blunder of epic proportions, and begs to be fixed.

    At the very least, another function is required to separate them.
     
    NotaNaN and Joe-Censored like this.
  13. brian1gramm1

    brian1gramm1

    Joined:
    Sep 15, 2017
    Posts:
    56
    Then why did Unity create a new Input System?

    Because the old system was not efficient enough.
     
    CunningFox146 likes this.
  14. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,924
    Except the new input system doesn't replace the old system. It exists alongside it, and does not break any projects using the old input system.
     
    Qriva likes this.
  15. brian1gramm1

    brian1gramm1

    Joined:
    Sep 15, 2017
    Posts:
    56
    Excellent point.

    OnDisabledOnly() should be the next new function...
     
  16. Extrys

    Extrys

    Joined:
    Oct 25, 2017
    Posts:
    345
    Any new about on disable only or a gameobject.ShouldDestroy / component.ShouldDestroy property?
     
  17. Qriva

    Qriva

    Joined:
    Jun 30, 2019
    Posts:
    1,314
    Around half year ago I decided to report issue about this topic (as "OnDisable do not follow destruction order"), it was accepted and shortly after I got message that it will be considered internaly, so I have no clue about current state.
    Also I asked if they can confirm if there are any rules of destruction (it seems there are probably for children), but it was ignored and I got nothing.

    Personally I just want to get expected behaviour - when OnDisable is called, there will be no objects destroyed on the same scene caused by destruction. Why I think it's expected? When Awake or OnEnable is called Unity ensures that all other objects are already deserialized and initialized, so I expect the same for OnDisable. Instead we are working in crumbling world.
     
    Extrys likes this.
  18. Xarbrough

    Xarbrough

    Joined:
    Dec 11, 2014
    Posts:
    1,188
    That's an unreasonable expectation because users can call Destroy or even DestroyImmediate whenever they want. It's an inconvenience that's present in other game engines and frameworks as well. But you can also solve the issue yourself if you have more information about your issue. For example, if you want to cleanup something before a scene transition, you can use that callback instead. What other reason for OnDestroy even comes to mind? It might be destroyed because it's part of a big hierarchy that is destroyed due to some external reason, again, if that isn't enough information you can always add your own IListenToUnloadCallback handler and call that. It's not like Unity would do much magic in the background to make these calls much more performant than what we can do in C#.

    Really, the whole range of issues described in this thread stem from the fact, that a general purpose game engine has a difficult time implementing user specific solutions. OnDisable and OnDestroy are merely side effects of other operations. Issues are different depending on which specific operation causes the OnDestroy. So we can't fix scene loading and object destruction and instantation with a single callback (unless Unity wanted to pass the context as some kind of argument, but that would be a maintenance nightmare). It makes more sense to deal with any issues at the level that knows about the context, e.g. if your scene loader script changes the scene, send a callback "OnBeforeSceneChange" if you need it to unload the game world in a specific way. The recommendation always is to keep scripts loosely coupled and modular, so in the best case they don't depend on each other. If scripts are coupled their interactions are better managed by a higher-level component (manager script) that knows about higher-level events like scene changes. Or, it's always possible to be pragmatic and check for null or check within the component itself whether it was already disabled or destroyed etc (just whatever state needs to be tracked).

    Sure, if enough users want more specific messages, Unity could implement them, but that would be going back to pre Unity 5 designs like OnLevelWasLoaded, which were removed later I assume because of multiple reasons. I for one would like Unity to stay as slim as possible and let me decide which callbacks to receive rather than knowning that the scripting engine has to process hundreds of messages even if I don't implement them.
     
  19. Qriva

    Qriva

    Joined:
    Jun 30, 2019
    Posts:
    1,314
    Your point of view is valid and I agree with you, however there are small nuances that can't be ignored.
    DestroyImmediate should not be used during runtime and Destroy is not only delayed to end of frame, but also OnDisable is called instantly. That means it's not possible that some object is going to disappear magically during that time.

    Imagine you have some manager or singleton on the scene. When you destroy the scene/quit game there are some scripts listening to events or calling managers and they have no clue if they are already destroyed. Yes, many of these cases can be fixed with some special checks or OnBeforeSceneChange or whatever is available, but it makes code more complex and it still can cause issue - what if there is event calling another script? You would need to nullcheck literally everything just in case.
    The point is you can take it into account and design your system in this way, but many of us can't do it as we are not aware - it's not described well by documentation and they don't provide solutions, plus on top of that it's hard to track this problem as destruction is random and in theory you might encounter this problem 0 times in your life and then your game crashes on production. Meanwhile it can be solved by ensuring all OnDisable calls before destroy, it happens only in one case and even if not, it can be done for normal Destroy call too (as it's delayed anyway).
     
  20. CunningFox146

    CunningFox146

    Joined:
    Mar 2, 2020
    Posts:
    18
    I mean, what's the problem with adding an option to disable calling OnDisable when object is destroyed? This is exactly what new input system does...
     
  21. Extrys

    Extrys

    Joined:
    Oct 25, 2017
    Posts:
    345
    I think that the best option from the unity's part would be adding a "ShouldDestroy" property to all monobehaviours
    as they from c++ has the full control on when they call the destruction, in that way on Disable could check if its disabling because is getting destroyed or because normal disabling
     
    MUGIK likes this.
  22. Kybernetik

    Kybernetik

    Joined:
    Jan 3, 2013
    Posts:
    2,570
    I also need a way to detect whether the object is being destroyed or not, whether a new method or a property to check.

    I have a component which performs a rather expensive operation to reset the object to its initial state if the component is disabled.

    But if the object is being destroyed then there's obviously no point in wasting performance on resetting things, but there doesn't seem to be any way for me to properly detect that.
     
    MUGIK likes this.
  23. jwlondon98

    jwlondon98

    Joined:
    Mar 27, 2015
    Posts:
    12
    the fact that this was ever a thing in the first place is ridiculous. its illogical. i get why they havent changed this, cus it would break other projects, but why not implement an additional version of OnDisable that only exclusively gets called when the object is disabled, and not when its destroyed? Call it OnDisableExclusive or something...

    I'd also like to add that forcing a developer to simply add a boolean or some other logic to handle the fact that OnDisable gets called at the same time as OnDestroy is not an acceptable solution to this issue. for my use case in particular I simply cannot add custom logic as it can be tampered with by cheaters.