Fix inability to use ResourceLoader in C# after threaded load in GDScript

[Hilderin]

Fixes #92798

As @raulsntos pointed out, the problem was from the modification of the default array. The hash of the method changed after the first call in GDScript.

I modified the method ResourceLoader::load_threaded_get_status to never modify the default array.

I search for DEFVAL(Array()) through all the code base and only found one another place where the default array was modified: OS::execute. I modified this method as well.

I was able to reproduce the problem and tested the correction with with the MRP project.

[AThousandShips]

I don't think this is an appropriate solution to this problem, this isn't the way to detect if it's the default argument as it's not just an empty array but a specific instance, you could do that by id on the Array but that'd require keeping a reference elsewhere for it

This will break compatibility, as the method is clearly intended to support empty arrays (or the resize wouldn't be there) so this requires a different solution

See:

• Arrays in default arguments can be modified #84407

[Hilderin]

You are so right @AThousandShips, sorry for that, still learning!! I was misleading big time by the DEFVAL, the == operator redefined in Array and the way default value were passed in GDScript.

As an alternative solution, I created a static _default_array class in ResourceLoader and OS and used it as default parameter for the method and ClassDB::bind_method. After that, I created the method is_same_instance in Array and used it to verify if the Array received is the default value.

What do you think?

[AThousandShips]

I don't think this is a valid solution either, this should be solved on the script and engine side not the local site IMO, it's occurring in various places probably

[Hilderin]

I understand that is not a perfect solution, but maybe a temporary solution? The alternative solution would be to have multiple overloads of the methods which seems a lot of work?!? Or maybe detect in GDScript that the default parameter is an empty array and create a new instance? That seems pretty specific.

Maybe you can point me in the direction for a better solution??

[AThousandShips]

I don't know a better solution sorry, this is not a simple problem, but this is a very ad-hoc and temporary solution in my opinion and I don't think it is an appropriate one, but I'll update the tags and we'll see what the core and GDScript teams say, but I feel this is too hacky to be a proper solution

[RandomShaper]

I don't know a better solution sorry, this is not a simple problem, but this is a very ad-hoc and temporary solution in my opinion and I don't think it is an appropriate one, but I'll update the tags and we'll see what the core and GDScript teams say, but I feel this is too hacky to be a proper solution

I've added a few style suggestions, just out of inertia. However, speaking about the issue itself, I totally agree with @AThousandShips. This needs to be discussed very carefully.

[Hilderin]

All the suggestions should be done. Let me known if more changes are needed.

[AThousandShips]

I'll try analyse this but I still think this isn't the way to solve this

[RandomShaper]

The issue is about optional output parameters. On a deeper thought, it may not be that bad to have the sort of idiom being used here for cases like this, where the bindings allow to default output parameters. As long as we are consistent, it may be a fix, at least short-term.

Ideally, though. the binding system would allow to be told a parameter is output so it automatically provides different instances if defaulted. That's pretty much the same this PR does, but making it more generic.

There may be other less error-prone alternatives that could be discussed.

[Hilderin]

Rebased to fix conflict with master.

[RandomShaper]

Reiterating a better solution may be worked on in the future, this is fine to me as it's clean enough and fixes a pretty unfortunate issue.

[akien-mga]

Thanks!