Godot 3.4.x breaks all prior versions of Area2D because it changes body from int to RID

[chucklepie]

Godot version

3.4.2

System information

Linux

Issue description

In all version of Godot the signature for body entered is this:

func _x(_body_id: int, _body: Node, _body_shape: int, local_shape: int) -> void:

In 3.4 it has changed to using RID

func _x(_body_id: RID, _body: Node, _body_shape: int, local_shape: int) -> void:

i.e. now using RID. This then fails to trigger any signals because the parameters do not match, so all signals fail to trigger.

I suspect everywhere that also changes to RID will also break too.

Observe the video.

RID.problem2.mp4

If this is known/documented, where is it and is there a list of all places where this/similar change will also break?

Steps to reproduce

1. Use a body entered area2d from 3.3
2. Debug and the method never calls
3. Change to RID manually and it works

Minimal reproduction project

No response

[Calinou]

Now that the compatibility breakage has been done, I'm not sure if it's worth reverting this change and breaking people's projects a second time in a short timespan.

[kleonc]

For reference it was changed in #42742 (#42743 for 3.x >= 3.4).

[chucklepie]

Got to be honest, the most fundamental principle of public API is don't change the interface, but if you do, make sure it's telegraphed loudly ;-)

Anyway, so it only affects Area2D, RigidBody and only area_shape_entered, area_shape_exited, body_shape_entered and body_shape_exited methods? My project has hundreds of scenes and signals, I expect it'll affect a lot of projects :(

Two things really then:

1. It would be nice for the changelogs/public notices to have a section (even if it says 'none') at the top to state what, if any, breaking changes are being made? This would give confidence to people upgrading.

2. I guess this shows one of the failing points of a lack of GDScript lint tool, or perhaps a suggestion could be that it should be possible for IDE created code to detect errors, i.e. given the signals are defined in the tscn file and the IDE highlights signals it should be fairly easy to show project warnings for this and similar errors, e.g. in this case signal is defined but the associated script has not defined it. At the minute it's happy as Larry showing the nice green connection symbol in the gutter while secretly laughing at you because it's fooled you into thinking it's connected...

3. Bonus! don't be helpful in signal generation by putting in data type hints, leave them as generic ;-)

[Calinou]

1. It would be nice for the changelogs/public notices to have a section (even if it says 'none') at the top to state what, if any, breaking changes are being made? This would give confidence to people upgrading.

While this sounds like a good idea on paper, my issue with this is that it prevents all changes from being nicely contained within a section (unless I repeat compatibility-breaking changes at the top, which can be confusing).

3. Bonus! don't be helpful in signal generation by putting in data type hints, leave them as generic ;-)

By default, type hints are not added when you connect a function in the editor. They are added because you enabled the editor setting that inserts type hints in autocompletion.

[chucklepie]

I guess then with 1 it was just overlooked because I defo checked the whole changelog to check for breaking issues. As for 2, I guess given nobody else has noticed signals failing completely on migrating, I must be the only person who has enabled type hints and/or thinks they're useful ;-)

…

On Sun, 2 Jan 2022 at 16:16, Hugo Locurcio ***@***.***> wrote: 1. It would be nice for the changelogs/public notices to have a section (even if it says 'none') at the top to state what, if any, breaking changes are being made? This would give confidence to people upgrading. While this sounds like a good idea on paper, my issue with this is that it prevents all changes from being nicely contained within a section (unless I *repeat* compatibility-breaking changes at the top, which can be confusing). — Reply to this email directly, view it on GitHub <#56410 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADCERRLRODDNKXBM4OSMAVTUUB26XANCNFSM5LC56SSQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[Giwayume]

I noticed, I thought it was an intentional change for 3.4.

[akien-mga]

It's not a bug and was indeed changed intentionally.

It's listed in the changelog for 3.4 in the Changed section, which includes things which changed and might indeed impact compatibility for existing projects. That's the section to look at if you're concerned about potential changes that might affect your project:

https://github.com/godotengine/godot/blob/3.4-stable/CHANGELOG.md#physics-1

Now it's indeed annoying that the type hints don't actually work here, as there should be one of two things in theory:

• Either the engine should report that the callback has the wrong signature - according to [3.x] Return RID instead of Object id in area-body_shape_entered-exited signals. #42743 (comment) that seems to be what it does at runtime
• Or it should report that the signal's callback doesn't exist at all (since changed signature == different method)

If it doesn't do that, it's a bug with GDScript, which might be fixed in 4.0 (type hints in 3.x are very barebones).

[chucklepie]

@akien-mga, It took a while but I found the entry stating the change you mentioned. Wasn't very clear it would break code, which was more my point about making breaking changes more prominent.

btw, you're right:
E 0:00:30.310 emit_signal: Error calling method from signal 'body_shape_entered': 'Camera2D(Flip_CameraFlip.gd)::_on_RoomNavigation_body_shape_entered': Cannot convert argument 1 from RID to int..
<C++ Source> core/object.cpp:1236 @ emit_signal()

I just never checked there, probably mainly because a combination of never noticing the error section and I'm that used to the errors all being benign as 99.9% of the time caused simply by a node being deleted then called in a yield. I think from my perspective I'd just like a lot more up-front static checking done, which I know is more difficult given the language.

btw, digressing one thing that always annoys me (another 4 fix I hope) is unless you have a scene file open and selected, when you have a script open you lose all '.' context help and all signal information like signal hints is gone. Would also be nice when inside a script to open the scene (or view list of scenes) that import this script to counter this problem. But I'm digressing :)

[akien-mga]

I'm that used to the errors all being benign as 99.9% of the time caused simply by a node being deleted then called in a yield.

FYI, that's definitely not a benign error, this would crash on release. Errors are usually things that you really need to fix before shipping a game. Godot does its best to recover from errors on debug so that you can fix them, but it won't be forgiving in an optimized release build.

[Xrayez]

3. Bonus! don't be helpful in signal generation by putting in data type hints, leave them as generic ;-)

Yeah, I know that the usage of GDScript type hints is heavily advertised (likely prolifirated by GDQuest tutorials), but you don't have to provide type hints for everything. I've previously stumbled upon similar errors in the past when updating Godot. By doing so, you can improve cross-compatibility.

My personal recommendation is to use type hints only when there's a definite ambiguity that must be resolved. Other than that, there's no benefit as of now. Using type hints won't improve performance either in 3.x (maybe in 4.0), otherwise I'd also recommend using type hints for performance critical parts of code after profiling it.

[Giwayume]

Adding type hints in the function signature helps with autocompletion, though.

[Xrayez]

Adding type hints in the function signature helps with autocompletion, though.

Yeah, but this may lead to situations where you find yourself using type hints for more than you actually need. I usually refer to documentation instead. Of course this is a personal preference, 🙂

I just wanted to convey that one should not blindlessly use type hints unless one really needs it. Typed GDScript is optional and should stay that way.

[chucklepie]

FYI, that's definitely not a benign error

@akien-mga I'm sure you know a lot more than me ;-), and maybe I wasn't explicit enough, but I'm just referring to the well known messages from Godot when a node is deleted but the code tries to resume back from yield. Godot just ignores this whether release or debug mode. It can't do anything else really because you can't code around a return to a yielded bit of code...