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

Update area_create description in PhysicsServer2D/3D to clear up possible confusions #91581

Conversation

TheKiromen
Copy link
Contributor

I'm a newbie who never worked with game engine before so apologies in advance if this is incorrect.

I spent way too long trying to figure out why collisions with areas created from Area2D nodes were working but not between areas created using PhysicsServer2D.area_create().

From my testing it looks like by default the monitorable property is set to false, which can be confusing since there is no easy way to check it (no area_get_monitorable() method for example). Especially since areas created with Area2D nodes have this enabled by default.

To reduce chances other people will be confused like me I updated the description of the method to explicitly state that.

I didn't mention monitoring property since areas are always monitoring, and disabling it in Area2D node just means setting empty callback. If you think there may be value in adding this as well, feel free to suggest changes.

@TheKiromen TheKiromen requested a review from a team as a code owner May 5, 2024 12:54
@AThousandShips AThousandShips added this to the 4.x milestone May 5, 2024
@AThousandShips AThousandShips requested a review from a team May 5, 2024 12:57
@rburing
Copy link
Member

rburing commented May 5, 2024

Looks good to me. Please copy the whole description to the method with the same name in PhysicsServer3D (and replace 2D by 3D), and I can approve the PR.

If you have time, please add the same information to body_create in both the 2D and 3D physics server.

@AThousandShips AThousandShips changed the title Update area_create description in PhysicsServer2D.xml to clear up possible confusions Update area_create description in PhysicsServer2D/3D to clear up possible confusions May 5, 2024
rburing
rburing previously approved these changes May 5, 2024
AThousandShips
AThousandShips previously approved these changes May 5, 2024
doc/classes/PhysicsServer3D.xml Outdated Show resolved Hide resolved
@rburing rburing dismissed their stale review May 5, 2024 16:48

Changes needed

@TheKiromen
Copy link
Contributor Author

Updated description for area_create() and body_create() int both PhysicsServer2D and 3D.

Since body doesn't have monitoring/monitorable properties I mentioned the default BodyMode there

@AThousandShips AThousandShips dismissed their stale review May 5, 2024 17:11

Needs new work

Copy link
Member

@rburing rburing left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@AThousandShips AThousandShips modified the milestones: 4.x, 4.3 May 5, 2024
@AThousandShips
Copy link
Member

Please squash your commits into one, see here

@TheKiromen TheKiromen force-pushed the improve-physics-server-documentation branch from c62ec9b to 445102d Compare May 5, 2024 17:30
@AThousandShips
Copy link
Member

Please clean up the commit message so it's just the necessary information, like the PR title, excluding all the "Update X" messages

@TheKiromen TheKiromen force-pushed the improve-physics-server-documentation branch from 445102d to 2b5e0d9 Compare May 5, 2024 17:40
@TheKiromen
Copy link
Contributor Author

I think I managed to do it, should I create a separate PR for body_create changes?

@akien-mga akien-mga changed the title Update area_create description in PhysicsServer2D/3D to clear up possible confusions Update area_create description in PhysicsServer2D/3D to clear up possible confusions May 6, 2024
@akien-mga akien-mga merged commit 608096e into godotengine:master May 6, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@TheKiromen TheKiromen deleted the improve-physics-server-documentation branch May 6, 2024 16:08
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.

None yet

4 participants