Change order of operation for C# types reloading

[paulloz]

• Relates to .NET: Failed to unload assemblies. Please check <this issue> for more information. #78513

The issue

With the following classes (granted they are used in a scene opened in editor), the reload process would trigger an error when restoring the value of RadialPatternResolver.RadialPattern. This happens because at the time the value is restored, we have not yet deserialized the managed callable associated with _radialPattern.Changed. Ultimately, our reload process would consider this a fail, and the infamous .NET: Failed to unload assemblies would show. Basically bricking the engine until we quit/restart.

[GlobalClass, Tool]
public partial class RadialPattern : Resource
{
    private bool _someToggle;

    [Export]
    public bool SomeToggle
    {
        get => _someToggle;
        private set
        {
            _someToggle = value;
            EmitChanged();
        }
    }
}

[Tool]
public partial class RadialPatternResolver : Node
{
    private RadialPattern _radialPattern;

    [Export]
    public RadialPattern RadialPattern
    {
        get => _radialPattern;
        private set
        {
            GD.Print($"RadialPattern setter (value={value}).");

            if (_radialPattern is not null)
            {
                _radialPattern.Changed -= OnRadialPatternChanged;
            }

            _radialPattern = value;

            if (_radialPattern is not null)
            {
                _radialPattern.Changed += OnRadialPatternChanged;
            }
        }
    }

    private void OnRadialPatternChanged()
    {
        GD.Print("OnRadialPatternChanged");
    }
}

The fix

We now deserialize callables before reloading property states, in case a property is doing anything with the callable in its getter and/or setter.

PSA

This should not change the behaviour currently implemented (apart from fixing the actual error), but it should be noted that said behaviour might be counter-intuitive to some users. In the example used above, after the reload, _radialPattern.Changed would be subscribed to twice (one is from before the reload, that we now properly restore, the other from during the reload, when the property's state is restored). We can see this is the currently implemented behaviour by connecting the signal with the deferred flag (this basically bypasses the original timing issue). I included this in the provided MRP.

This is probably not ideal, but we probably don't want to widely change the behaviour here. Users can implement the ISerializationListener if they need finer control.

Minimal reproduction

• MRP.zip

[raulsntos]

I think this makes sense, although I agree that the double subscription is not ideal. It seems like a niche use case though, since it would only affect connections done in serialized properties.

Still, it would be great to get some user testing so we can evaluate how disruptive this change is, maybe since we have ISerializationListener as a escape hatch it may be good enough.

Thanks!

[paulloz]

I agree that the double subscription is not ideal

I'm wondering if we could find a decent solution for it. But it really isn't a trivial problem, and like we already discussed several times, I'm more interested in stabilizing what is currently here, than anything else ATM 🤔

[akien-mga]

Thanks!