godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.85k stars 20.15k forks source link

Misleading return value on Object::call_deferred #95491

Open RadiantUwU opened 1 month ago

RadiantUwU commented 1 month ago

Tested versions

master

System information

N/A

Issue description

Call deferred is marked as returning Variant while it actually always returns null. This is misleading as it may confuse developers into thinking it can return something else. Easily patchable by changing the method info in object.cpp Screenshot_20240813_185246_GitHub

Steps to reproduce

N/A

Minimal reproduction project (MRP)

N/A

RadiantUwU commented 1 month ago

Will open a PR to resolve thus when i get back home!

AThousandShips commented 1 month ago

The documentation already makes this quirk clear:

Always returns null, not the method's result.

sockeye-d commented 1 month ago

To be clear the issue is not in the documentation, which clearly explains that it returns null. It's that someone who used it assuming it would return something useful would be very confused when it didn't

AThousandShips commented 1 month ago

Then this issue is named incorrectly? But we can't change anything else about it, changing the return value would be a major change

RadiantUwU commented 1 month ago

Then this issue is named incorrectly? But we can't change anything else about it, changing the return value would be a major change

Do we have other things that rely on this being Variant except of GDExtension?

AThousandShips commented 1 month ago

The reason this returns Variant is probably for some legacy reason and might just be a leftover, however this is a very core method and I don't think it would be reasonable to change it, it might seriously impact things downstream in various cases

But it's something to discuss, but the fact is documented and I think there's little justification for breaking compatibility for this

RadiantUwU commented 1 month ago

The reason this returns Variant is probably for some legacy reason and might just be a leftover, however this is a very core method and I don't think it would be reasonable to change it, it might seriously impact things downstream in various cases

But it's something to discuss, but the fact is documented and I think there's little justification for breaking compatibility for this

Why didnt we change it in 4.0 release? This is likely to break compat with scripts that havent found out you can't await on call_deferred as it returns null.

AThousandShips commented 1 month ago

Why didnt we change it in 4.0 release?

I just assumed it was for that reason, don't know that it is (hence the "probably" and "might"), but the reason for it matters very little

But I also don't expect many people to have a problem with this, I don't see what someone would expect to return from this method?

RadiantUwU commented 1 month ago

But I also don't expect many people to have a problem with this, I don't see what someone would expect to return from this method?

They expect that the function would return something like a GDScriptFunctionState (that we cannot add since gdscript isnt core), which would give them the return value of the deferred call. Or a signal to await on to give them the value of the deferred call.

AThousandShips commented 1 month ago

They expect that the function would return something like a GDScriptFunctionState

That's an assumption they can't rely on as that's not how the await system works as of 4.x (and assuming it based on more complex functionality would be easily dispelled by reading the documentation)


But beyond people being a bit confused if they don't read the documentation, what is the actual problems caused by assuming something about the return value?

I agree that this can be confusing, and if this was during the 4.0 development and there wasn't some specific reason to keep this, I'd say it should have been changed, but now I really don't think it's justified

Edit: digging through this method always returned Variant(), ever since the first open source release, so the change is likely somewhere in the way methods are bound, as it doesn't return anything in the 3.x documentation

Some context:

So seems something might have broken along the way for that method processing, unsure

But it's unclear if the return of Variant is intentional or just a limitation, I'd say it leans to the latter even though the code was different in 3.2, but it's fully safe to use it, and the return value is valid, even though it's just null, so the impact is, IMO, limited of leaving it as is, and trying to fix it would be more complicated and would risk introducing any number of bugs given how core this feature is (it'd be part of the system for bindings, which is really fundamental) even if we consider the compatibility breakage acceptable

So this should probably be a broader investigation of what might have broken and seeing if it can be solved with minimal risk and if it's worth it for compatibility