Trouv / bevy_ecs_ldtk

ECS-friendly ldtk plugin for bevy, leveraging bevy_ecs_tilemap
Other
629 stars 74 forks source link

"overflow" layer_entities too large #227

Closed MScottMcBee closed 9 months ago

MScottMcBee commented 10 months ago

Expected Result

Given a 10x10 int_grid named "map" with 10 tiles in one space and one tile in the remaining spaces, spawn_level should spawn 1 entity named "map" with 100 tiles, and 9 additional "map" entities each with 1 tile each to cover the "overflow"

Observed Result

Given the same map as above, spawn_level will spawn 1 "map" entity with the correct 100 tiles, and then the 9 overflow map entities will also have 100 tiles each, 99 of which are invisible tiles. All the invisible tiles can take a big toll on performance and memory in larger levels.

Suspected cause

layer_grid_tiles splits the tiles into correct grid_tiles Vecs, tile_pos_to_tile_if_int_grid_nonzero_maker checks against the whole int_grid_csv array on where to create a tile, and not grid_tiles, so an invisible tile is created where ever a tile exists on any layer_entity

I've made a fix for this issue in my fork, and I've included a PR for it here: https://github.com/Trouv/bevy_ecs_ldtk/pull/228

MScottMcBee commented 9 months ago

I spent some more time on this issue. I made a small level in LDTK to reproduce it, and it got messy quick. Default options in LDTK prevent the issue from happening in a lot of cases and #228 is broken in a few ways, mostly in how I set coordinates. In working through the issues, though, I've come to believe that creating invisible tiles for Auto-layer layers doesn't serve a purpose in the most common use of LDTK.

I realize this opinion does have a decent amount of ignorance behind it (I'm not the one who's been working on this library for years), but I've submitted a PR for consideration anyway. Feel free to disregard.

Trouv commented 9 months ago

To elaborate on the purpose of invisible tiles in general - it's pretty much exclusively to ensure that intgrid cells will spawn appropriately for intgrid+autotile layers. Intgrid values tend to be pretty important to game logic and the APIs provided by this plugin. So, on intgrid+autotile layers, tiles with a nonzero intgrid value need to exist whether or not the autotile rules of that layer determine that they have a tileset value. Conversely, the tile needs to exist (and be visible) on intgrid+autotile layers if it has a tileset value but happens to have a 0 intgrid value.

That being said, the issue of the overflow layers also containing these tiles is a definitely a bug. Only the bottom sub-layer should be concerned with int grid logic, so spawning invisible tiles for them on the remaining sub-layers is wrong and it was a good catch.

Haven't checked out your PR yet btw - just giving some extra context here

MScottMcBee commented 9 months ago

Thank you for the context, that makes a lot of sense. Based off that, I've tweaked my approach and opened up a new PR here: #231

Trouv commented 9 months ago

Implemented in #231