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

Fix panic when despawning tiles that have changed #384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Commander-lol
Copy link

Right now, when you despawn a tile that has had one of its bevy_ecs_tilemap specific components changed in the same frame, your game will panic.

I tracked the behaviour down to the logic extracting tile info for rendering, where a get on a Query is unwrapped. The fix, instead, just skips extracting the tile in question if the entity for it has been despawned - it occurs before any of the mutations, so there shouldn't be any dangling data left over by continueing. I haven't added a log for the Err, since it seems like the only time this unwrap fails is when the entity can't be found in the query, which should be unexceptional.

Changing levels with the patched version - water tiles are animated by changing the `TileTextureIndex`, which causes the panic when the `TileStorage` for the animated layer is despawned
2023-01-16.03-25-56.mp4

@StarArawn
Copy link
Owner

I'm not sure this is the correct approach to solving this issue. What the error here suggests to me is that a tile was not despawned when the tilemap it is apart of was despawned. I think a better fix is to make sure we attach tiles to the tilemap correctly(using bevy's hierarchy). None of the examples show this but we really should be calling:
https://docs.rs/bevy/latest/bevy/hierarchy/trait.BuildChildren.html#tymethod.set_parent

@Commander-lol
Copy link
Author

Ah, that would also make sense. I tried a couple of different variations, and it does look like it's not despawning the tile entities (even when I iterate over the TileStorage and call despawn on each one...), so this PR totally misses the mark

Using Bevy's Hierarchy does work - I added commands.entity(tile_entity).set_parent(layer_entity); to the level spawning code to try it out, and that also fixes the issue (without the code in this PR)

Before I close this and open a new PR, would you be on board with adding a new system that does this when TileStorage components are changed?

The system that I wrote locally to do this looks like this:

fn set_tile_parents(mut commands: Commands, query: Query<(Entity, &TileStorage), Or<(Changed<TileStorage>, Added<TileStorage>)>>) {
    for (layer_entity, tile_storage) in &query {
        for tile_entity in tile_storage.iter() {
            if let Some(tile_entity) = tile_entity {
                commands.entity(*tile_entity).set_parent(layer_entity);
            }
        }
    }
}

@StarArawn
Copy link
Owner

StarArawn commented Jan 16, 2023

Ah, that would also make sense. I tried a couple of different variations, and it does look like it's not despawning the tile entities (even when I iterate over the TileStorage and call despawn on each one...), so this PR totally misses the mark

Using Bevy's Hierarchy does work - I added commands.entity(tile_entity).set_parent(layer_entity); to the level spawning code to try it out, and that also fixes the issue (without the code in this PR)

Before I close this and open a new PR, would you be on board with adding a new system that does this when TileStorage components are changed?

The system that I wrote locally to do this looks like this:

fn set_tile_parents(mut commands: Commands, query: Query<(Entity, &TileStorage), Or<(Changed<TileStorage>, Added<TileStorage>)>>) {
    for (layer_entity, tile_storage) in &query {
        for tile_entity in tile_storage.iter() {
            if let Some(tile_entity) = tile_entity {
                commands.entity(*tile_entity).set_parent(layer_entity);
            }
        }
    }
}

It's usually better to associate the tiles with the tilemap on tile spawn. I think a better way is to use bevy's with_children. There are some performance benefits to using that method as it allocates more quickly or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants