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

"overflow" layer_entities too large #227

Closed
MScottMcBee opened this issue Sep 12, 2023 · 4 comments
Closed

"overflow" layer_entities too large #227

MScottMcBee opened this issue Sep 12, 2023 · 4 comments

Comments

@MScottMcBee
Copy link
Contributor

MScottMcBee commented Sep 12, 2023

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: #228

@MScottMcBee
Copy link
Contributor Author

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
Copy link
Owner

Trouv commented Sep 18, 2023

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
Copy link
Contributor Author

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 pushed a commit that referenced this issue Sep 19, 2023
…d layers (#231)

Another possible solution to #227. Based off of the previous settings
PR, this changes it so that instead of passing a setting to spawn
invisible tiles or not, it passes the sublayer index.
@Trouv
Copy link
Owner

Trouv commented Sep 19, 2023

Implemented in #231

@Trouv Trouv closed this as completed Sep 19, 2023
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

No branches or pull requests

2 participants