Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[.NET] Fix serialization of delegates capturing variables #83217

Conversation

paulloz
Copy link
Member

@paulloz paulloz commented Oct 12, 2023

WIP fix for #81903.

Let's use a similar example to the one described in the issue.

[Tool]
public partial class Scene : VBoxContainer
{
    public override void _Ready()
    {
        for (var i = 0; i < 4; ++i)
        {
            var button = new Button { Text = $"Click me!" };

            var buttonIndex = i;

            // Case 1
            button.Connect(Button.SignalName.Pressed, Callable.From(() => GD.Print($"Button #{buttonIndex} was clicked!"));
            // Case 2
            button.Connect(Button.SignalName.Pressed, Callable.From(() => WhenButtonClicked(buttonIndex)));

            AddChild(button);
        }
    }

    private void WhenButtonClicked(int buttonIndex)
    {
        GD.Print($"Button #{buttonIndex} was clicked!");
    }
}

As of right now, this PR completely fixes case 1 (crudely: captured variables aren't GodotObject).

On my way to fixing case 2 (here, we also capture this). I had to switch the serialization and call godotsharp_var_to_bytes() with p_full_objects = true. I'm not entirely sure about it, there might be security concerns? Everything now serializes fine (and thus, is properly unloaded). But once we try to deserialize the delegate back, this (in the example above) is considered as a VBoxContainer instead of the actual concrete class, resulting in an ArgumentException when we try to use its value. I kinda imagine it's because of the script being unloaded in between, but I can't really think of any good/simple solution here.

Hoping for someone to have a stroke of genius, or to rubber duck me in the right direction ❤️

I've also been wondering on and off if serializing these when reloading is actually something we want to do in this case.

@Calinou Calinou added this to the 4.2 milestone Oct 12, 2023
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect closures weren't really tested thoroughly when this was implemented. This is not an area of the module that I'm very familiar with, so I'm trying to understand why it was implemented this way.

But there are definitely improvements in this PR compared to the current implementation, so if it fixes some cases I think that may be worth merging.

I've also been wondering on and off if serializing these when reloading is actually something we want to do in this case.

I also wonder if serializing these is necessary at all.

@raulsntos raulsntos modified the milestones: 4.2, 4.3 Oct 27, 2023
@raulsntos raulsntos added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Oct 27, 2023
@paulloz paulloz changed the title .NET: Fix serialization of delegates capturing variables [.NET] Fix serialization of delegates capturing variables Feb 18, 2024
@paulloz paulloz force-pushed the fix/81903-delegate-capture-serialization branch from 0d5fd38 to c542931 Compare March 5, 2024 21:58
@paulloz paulloz force-pushed the fix/81903-delegate-capture-serialization branch from c542931 to c310ecc Compare April 4, 2024 16:59
@paulloz paulloz removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 4, 2024
@paulloz
Copy link
Member Author

paulloz commented Apr 4, 2024

I'm removing the cherrypick:4.2 tag, because this can't be cherry-picked without #78219.

@paulloz paulloz marked this pull request as ready for review April 4, 2024 17:01
@paulloz paulloz requested a review from a team as a code owner April 4, 2024 17:01
@Chubercik
Copy link
Contributor

If it's ready for review, it might be worth it to update the PR description :)

@paulloz
Copy link
Member Author

paulloz commented Apr 4, 2024

If it's ready for review, it might be worth it to update the PR description :)

I think the actual back and forth in the comments is easier to understand than re-writing the history?

Long story short:

  • We used to try to serialize a System.Type instead of our own representation of variant type like everywhere else.
  • We used to have legacy code relying on IConvertible, but no Godot object implements that.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@akien-mga akien-mga merged commit 72f9f8d into godotengine:master Apr 5, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Capturing a variable in a Callable prevents assembly unloading
5 participants