Skip to content

Prevent mapping areas with invalid IDs for Area2D/3D - #79460

Merged
YuriSizov merged 1 commit into
godotengine:masterfrom
AThousandShips:area_object
Dec 18, 2023
Merged

Prevent mapping areas with invalid IDs for Area2D/3D#79460
YuriSizov merged 1 commit into
godotengine:masterfrom
AThousandShips:area_object

Conversation

@AThousandShips

@AThousandShips AThousandShips commented Jul 14, 2023

Copy link
Copy Markdown
Member

The inout code of area assumes a valid ObjectID is attached to all bodies and areas, leading to it mapping the invalid one to one entry, which doesn't make sense and creates strange behaviour

Added a separate commit for emitting the signals, uncertain if they should or should not be emitted in these cases, there's also an inconsistency between bodies between 2D and 3D which I matched there, can omit the signals if desired

Will also look into the same for bodies, and 3.x

@AThousandShips AThousandShips added this to the 4.2 milestone Jul 14, 2023
@AThousandShips
AThousandShips requested a review from a team July 14, 2023 11:05
@AThousandShips
AThousandShips requested review from a team as code owners July 14, 2023 11:05
@AThousandShips AThousandShips changed the title Area object Prevent mapping areas with invalid IDs for Area2D/3D Jul 14, 2023
Comment thread doc/classes/Area2D.xml Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[b]Note:[/b] [Area2D]s and [PhysicalBody2D]s created..

Also, is this correct? This makes it sound like creating them from PhysicsServer would result in instability. Is this only at the very beginning of simulation or?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is correct, they are not associated with a node so they work differently,

Comment thread scene/3d/area_3d.cpp Outdated
@golfinq

golfinq commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

Maybe add a note to the physics servers docs as well?

@AThousandShips

AThousandShips commented Oct 24, 2023

Copy link
Copy Markdown
Member Author

This is specifically about behaviour of Area2D/3D and not the server ones, so I think it isn't relevant in the server as the areas created three don't work the same way, and don't have these signals etc. and therefore unless you read the documentation for the nodes you won't really care about those parts

The server side description is lacking but IMO outside the scope of this fix

@golfinq

golfinq commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

Makes sense, thanks for explaining

@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 28, 2023
@AThousandShips AThousandShips added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Oct 28, 2023
Comment thread scene/2d/area_2d.cpp Outdated
Comment thread scene/2d/area_2d.cpp Outdated
Comment thread scene/3d/area_3d.cpp Outdated

@YuriSizov YuriSizov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can squash commits, emitting here seems to follow the established logic. That logic is kind of weird and indeed can lead to the signal being emitted with some garbage object reference, but I don't think your changes are going to make it worse in any way.

@AThousandShips
AThousandShips force-pushed the area_object branch 2 times, most recently from 061ca8b to 1e11932 Compare December 18, 2023 14:58
This occurs when areas are created directly from the servers, and no
instance is linked.

@YuriSizov YuriSizov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense to me

@YuriSizov
YuriSizov merged commit 612791e into godotengine:master Dec 18, 2023
@YuriSizov

Copy link
Copy Markdown
Contributor

Thanks!

@AThousandShips

Copy link
Copy Markdown
Member Author

Thank you!

@AThousandShips
AThousandShips deleted the area_object branch December 18, 2023 17:26
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
Prevent mapping areas with invalid IDs for `Area2D/3D`
BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
Prevent mapping areas with invalid IDs for `Area2D/3D`
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.

Node-based Area3Ds can't tell when overlapping with PhysicsServer instanced ones.

5 participants