search

Trouv / bevy_ecs_ldtk

ECS-friendly ldtk plugin for bevy, leveraging bevy_ecs_tilemap
Other
627 stars 73 forks source link

Add exclusions to `LdtkSettings` and filters to level spawner. #275

Closed focustense closed 7 months ago

focustense commented 7 months ago

I made this a separate LdtkExclusions struct because, even though there's only one use case for this now, it would be a logical extension to add other exclusions in the future (like entities, both IID and UID based), and if so, it's better to group them together instead of cluttering up the main LdtkSettings.

Caveat: Adding a Vec (or any other collection) to the LdtkSettings or any sub-structure means we can no longer derive Copy. It doesn't appear to be required by any part of bevy_ecs_ldtk itself, and there's no compelling reason why users should absolutely need it (copying a Resource is pretty rare?), but this is potentially a breaking change for some users. The only way around this would be to avoid putting this in LdtkSettings altogether and define the LdtkExclusions as its own resource (and modify the various systems accordingly). I feel that's a YAGNI problem so I went the easy route and just removed Copy.

Unit testing seems infeasible at the moment due to the complexity of the spawn_level method and absence of existing test infra for it, but I manually tested the exact same change on the 0.8 branch in an actual game and it worked perfectly.

Note also that the use of Vec over HashSet is intentional. I'd expect there to be a very small number of items in that list (say 2 or 3). In lists with fewer than 10-15 items, the cost of the hash tends to exceed the cost of the iteration (example).

Fixes #272

Trouv commented 7 months ago

Thanks for the contribution!

I made this a separate LdtkExclusions struct because, even though there's only one use case for this now, it would be a logical extension to add other exclusions in the future

Nice, this is definitely an improvement over my original suggestion

I feel that's a YAGNI problem so I went the easy route and just removed Copy.

agreed

Unit testing seems infeasible at the moment due to the complexity of the spawn_level method and absence of existing test infra for it

That's fine. I plan to rewrite this plugin's systems to be smaller/more modular eventually, and when that time comes I will have tests for them and begin asking contributors for tests as well. For now they're definitely unreasonably difficult to test.