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 tilemap physics' World2D on reparenting #84968

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

groud
Copy link
Member

@groud groud commented Nov 16, 2023

Supersedes #84757
Fixes #83291

This is the same fix as in #84757, with the changes I requested. Made this superseding PR to have this included in RC1

@groud groud requested a review from a team as a code owner November 16, 2023 10:29
@groud groud added this to the 4.2 milestone Nov 16, 2023
scene/2d/tile_map.cpp Outdated Show resolved Hide resolved
Co-authored-by: Alon Ran <newdefectus@gmail.com>
@@ -686,26 +686,30 @@ void TileMapLayer::_physics_update() {

void TileMapLayer::_physics_notify_tilemap_change(TileMapLayer::DirtyFlags p_what) {
Transform2D gl_transform = tile_map_node->get_global_transform();
PhysicsServer2D *ps = PhysicsServer2D::get_singleton();
Copy link
Member

@AThousandShips AThousandShips Nov 16, 2023

Choose a reason for hiding this comment

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

Don't know that storing the server in a variable for three uses is necessary and makes the code harder to read IMO (leftover from the original PR and was planning to comment the same there)

Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree it's not necessary, but it doesn't seem to be a bad change either, so I'm fine merging as is (and mostly in a hurry to get the master branch somewhat RC1 ready ;)).

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know that storing the server in a variable for three uses is necessary and makes the code harder to read IMO (leftover from the original PR and was planning to comment the same there)

It's commonly done in the TileMap codebase (using ps, rs or ns for the different servers), so it's more or less consistent with what we already have I believe. I think anyone familiar with the TileMap-related code should be familiar with this, so it's does not hurt readability much I think. But yeah, that's debatable.

@akien-mga akien-mga merged commit 4065266 into godotengine:master Nov 16, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks @NewDefectus and @groud!

@akien-mga akien-mga changed the title Update tilemap physics' world2D on reparenting Update tilemap physics' World2D on reparenting Nov 17, 2023
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.

Reparenting a CharacterBody2D/TileMap duo to or from a SubViewport disables their collisions
3 participants