ProwlEngine / Prowl

An Open Source C# 3D Game Engine under MIT license, inspired by Unity and featuring a complete editor and built on Silk.NET
MIT License
360 stars 30 forks source link

Fixed Issue Where Objects Didn't "Turn Off" or "On" Reliablity #51

Closed 10xJosh closed 9 months ago

10xJosh commented 9 months ago

Howdy! This pull request is for issue #30 . It turns out that the issue is caused because there are multiple objects being stored within an object.

The objects can be found in red in the following screenshot. 2023-11-24 01_18_33-Prowl (Debugging) - Microsoft Visual Studio

The "Enabled" property was only being set for the selected item and not the "lower" or "higher" objects in the hierarchy. The code I'm pushing through iterates through each of the objects and sets all objects to be either enabled or disabled for both the Hierarchy View and the Inspector view, when the respective boxes are checked

michaelsakharov commented 9 months ago

Hey! Thanks for looking into this!

Game objects and components have a HierarchyStateChanged() method, which when a parent's enabled state changes it should iterate over children to update them. This is done through the Enabled Setter property.

We shouldn't need to do that ourselves it should be automatic.

GameObject.Enabled; GameObject.SetEnabled(bool state); GameObject.HierarchyStateChanged();


public bool Enabled
{
    get { return _enabled; }
    set { if (value != _enabled) { SetEnabled(value); } }
}

private void SetEnabled(bool state)
{
    _enabled = state;
    foreach (var child in Children)
        child.HierarchyStateChanged();
    HierarchyStateChanged();
}

private void HierarchyStateChanged()
{
    bool newState = _enabled && IsParentEnabled();
    if (_enabledInHierarchy != newState)
    {
        _enabledInHierarchy = newState;
        foreach (var component in GetComponents<MonoBehaviour>())
            component.HierarchyStateChanged();
    }
}

So just setting Enabled = false; should automatically update all children GameObjects & Components. But this is what doesn't appear to be functioning as intended.

10xJosh commented 9 months ago

Thanks! Still working on this. Its weird because even though the selected item is set to false it just won't change the remaining children. I need to tinker with it a bit more to narrow it down. Also when the "GetComponents" gets triggered, it sometimes doesn't pass the components of the objects, I think thats what is likely causing the issue. Gonna look more into it in a bit.

10xJosh commented 9 months ago

Game objects and components have a HierarchyStateChanged() method, which when a parent's enabled state changes it should iterate over children to update them. This is done through the Enabled Setter property.

I didn't realize what you ment by this until later.. sorry about that.

So just setting Enabled = false; should automatically update all children GameObjects & Components. But this is what doesn't appear to be functioning as intended.

From what I saw in the code, there wasn't any place where the parent/child was being set besides the original one that was clicked. Whats interesting is that the only time the mesh would be disabled was when the base object at the bottom was hidden. Whenever I hit the "GetComponents", it would only count the base object as an gameobject and not the other stuff in the hierarchy. I think this is also what added to some of the randomness of hiding the stuff.

The code I wrote has fixed the issue so that whenever something gets clicked, it will hide everything and also will be ticked as disabled in the inspector window as well. Let me know if theres anything you want changed or if I completely missed the mark here.

I removed the loops in Prowl.Editor/EditorWindows/CustomEditors/GameObjectEditor.cs and in Prowl.Editor/EditorWindows/HierarchyWindow.cs as well.

michaelsakharov commented 9 months ago

Hmm, I think i might have explained issue #30 poorly, heres some examples of the issue and some gameobject structures you can use to test with:

Heres some GameObjects, A B, and C A is toggled off, this is working as intended in the original version: image All of them are greyed out their 'EnabledInHierarchy' values are false because their root object A is turned off.

Heres an example of it failing: image D is enabled, even though the root A is disabled.

Heres another Example: image A is disabled and G is disabled. The only ones that should be enabled are F and J the rest are turned off, yet I, D, and E are enabled.

G being turned off should not turn off F, which is the case in your code.

10xJosh commented 9 months ago

Will continue to look into this

10xJosh commented 9 months ago

Ok, issue should be fixed. Let me know if theres anything that needs to be changed!

michaelsakharov commented 9 months ago

Closer, this, unfortunately, changes the actual Enabled state of children, only the EnabledInHierarchy should be changing since they're not being disabled their parents are.

The issue is its not recursively going down properly to disable all the children objects, you have the right idea going through all children and disabling them. But the recursive method should already do that, thats where the issue is.

    private void SetEnabled(bool state)
    {
        _enabled = state;
        foreach (var child in Children)
            child.HierarchyStateChanged();
        HierarchyStateChanged();
    }

SetEnabled loops over and updates the EnabledInHierarchy state of all the children before updating our own, and HierarchyStateChanged itself doesn't update children, so this only goes 1 layer deep.

So that foreach should be moved to the bottom of HierarchyStateChanged and removed from SetEnabled(), this way it happens after we update our EnabledInHierarchy, and it calls it again automatically for all children:


    private void HierarchyStateChanged()
    {
        bool newState = _enabled && IsParentEnabled();
        if (_enabledInHierarchy != newState)
        {
            _enabledInHierarchy = newState;
            foreach (var component in GetComponents<MonoBehaviour>())
                component.HierarchyStateChanged();
        }

        foreach (var child in Children)
            child.HierarchyStateChanged();
    }

But there's one more issue, IsParentEnabled() checks if it's Enabled, not EnabledInHierarchy, so the issue will probably persist.

    private bool IsParentEnabled()
    {
        return Parent == null || Parent.Enabled; // Should be EnabledInHierarchy
    }

Hope that helps!

10xJosh commented 9 months ago

Thank you for the explanation and thank you for your patience with me on this. I think I found why the objects arent being disabled like they should.

I was looking at the code and I'm guessing that the foreach loop in the HierarchyStateChanged() function is what is being used to enable/disable the IsEnabled property for each of the children

         if (_enabledInHierarchy != newState)
        {
            _enabledInHierarchy = newState;
            foreach (var component in GetComponents<MonoBehaviour>())
                component.HierarchyStateChanged();
        }

This foreach loop also doesn't get all of the children that are in the hierarchy only the ones with actual objects attached to them. Whats interesting is that instead of getting the object itself and converting it, it creates an entirely new object into a MonoBehaviour object and adds the GameObject inside.

2023-11-26 19_29_22-Prowl (Debugging) - Microsoft Visual Studio

2023-11-26 19_31_00-Prowl (Debugging) - Microsoft Visual Studio

Using component.GameObject.HierarchyStateChanged(); doesn't work because it calls the HierarchyStateChanged() within the GameObject class and not the MonoBehaviour/Component class. Since the MonoBehaviour object is completely new, it passes the enable check statements in the HierarchyStateChanged() function.

2023-11-26 19_45_21-Prowl (Debugging) - Microsoft Visual Studio

Going to look more into whats going on but this is where I'm at so far

michaelsakharov commented 9 months ago

There is a Different HierarchyStateChanged for Components, the _go.Enabled in that Component.HierarchyStateChanged should probably use _go.EnabledInHierarchy.

GetComponents only gets the Components attached to a particular game object, not its children in the hierarchy, that's intended.

michaelsakharov commented 9 months ago

I think the code i sent before should fix most of the issues.

10xJosh commented 9 months ago

I think the code i sent before should fix most of the issues.

Yes it did for the initial issue with the hierarchy not being hidden properly. I updated it in case you want to merge it