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 space on tree notifications #84757

Closed
wants to merge 1 commit into from

Conversation

NewDefectus
Copy link
Contributor

@NewDefectus NewDefectus commented Nov 11, 2023

Fixes #83291. As mentioned in that issue, the relevant problem (collision bugginess when reparenting TileMaps to SubViewports) first appeared in #81070. I've tried researching the cause of this regression and why it didn't occur before that commit, but I'm not too sure honestly. So this fix doesn't directly tackle the issue of why this needs to be done at all, especially why this only occurs for SubViewports, but it definitely works and I think it's elegant enough not to cause any other issues.

@AThousandShips
Copy link
Member

CC @groud

@YuriSizov YuriSizov requested review from KoBeWi and groud November 13, 2023 13:45
@groud
Copy link
Member

groud commented Nov 14, 2023

Hmm, I've just checked. I am not sure this is the right fix. Technically, the re-creation of all physics shapes should already happen thanks to _physics_was_cleaned_up being true.

I suspect the issue would be due to RID space = tile_map_node->get_world_2d()->get_space();. As in a subviewport, the expected space might change.

I think the solution would be to instead update _physics_notify_tilemap_change to update the bodies space on DIRTY_FLAGS_TILE_MAP_IN_TREE change or something. That would avoid recreating all the physics shapes.

@NewDefectus
Copy link
Contributor Author

Hmm, I've just checked. I am not sure this is the right fix. Technically, the re-creation of all physics shapes should already happen thanks to _physics_was_cleaned_up being true.

I suspect the issue would be due to RID space = tile_map_node->get_world_2d()->get_space();. As in a subviewport, the expected space might change.

I think the solution would be to instead update _physics_notify_tilemap_change to update the bodies space on DIRTY_FLAGS_TILE_MAP_IN_TREE change or something. That would avoid recreating all the physics shapes.

Yeah, I figured this wasn't the optimal solution - what you say about the space changing makes more sense. I'll replace the commit.

@NewDefectus NewDefectus changed the title Flush tilemap physics on tree notifications Update tilemap space on tree notifications Nov 14, 2023
@groud
Copy link
Member

groud commented Nov 15, 2023

I think it's the right solution, but I kind of dislike the refactor you added with the other notifications. I understand why you would like to change it that way but it makes the code a lot harder to read IMO.

Animatable collisions is kind of an easy thing to mess up (it basically does a complex thing, like reverting transforms changes in _process then re-applying them later on). So I'd rather keep it easier to read, even it it implies a few more lines.

@akien-mga
Copy link
Member

Superseded by #84968, which is your fix with the additional changes requested (speed tracked to include it in RC1 today).

@akien-mga akien-mga closed this Nov 16, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Nov 16, 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
4 participants