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

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

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

focustense
Copy link
Contributor

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

Trouv commented Dec 5, 2023

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.

Copy link
Owner

@Trouv Trouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor notes

src/resources/mod.rs Outdated Show resolved Hide resolved
src/resources/mod.rs Outdated Show resolved Hide resolved
@focustense focustense requested a review from Trouv December 6, 2023 00:32
@Trouv Trouv merged commit 282404d into Trouv:main Dec 6, 2023
5 checks passed
@Trouv Trouv mentioned this pull request Dec 6, 2023
Trouv added a commit that referenced this pull request Feb 11, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.9.0](v0.8.0...v0.9.0)
(2024-02-11)


### ⚠ BREAKING CHANGES

* upgrade to bevy 0.12
([#265](#265))
* upgrade to LDtk 1.5.3, dropping support for previous versions
([#295](#295))
* add `SpawnExclusions` to `LdtkSettings` for skipping layers by
identifier ([#275](#275))
* add layer entity for Entity layers, changing the hierarchy
([#257](#257))
* upgrade to LDtk types and examples to 1.4.1 (drop support for <1.4.1)
([#256](#256))
* LdtkLevel renamed to LdtkExternalLevel and is no longer used as a
component ([#244](#244))
* redesign LdtkProject with better level data accessors and correct
modeling of internal/external levels
([#244](#244))
* use the bundle's `Default` implementation rather than the field's in
`LdtkEntity` and `LdtkIntCell` derive macros
([#222](#222))
* add `RawLevelAccessor` trait for `LdtkJson` level borrowing/iteration,
replacing existing methods
([#225](#225))
* add `LevelIndices` type defining a level's location in a project and
use it in `LevelSelection::Indices`
([#221](#221))
* change `LevelEvent` inner types from `String` to `LevelIid`
([#219](#219))
* change `LevelSet` inner type from `String` to `LevelIid`
([#219](#219))
* change `LevelSelection::Iid` inner type from `String` to `LevelIid`
([#219](#219))
* replace `LevelSet::from_iid` with `LevelSet::from_iids`, which can
convert from any collection of strings.
([#219](#219))
* use new LevelIid type in LevelEvent, LevelSet, and LevelSelection,
plus other improvements
([#219](#219))
* `LdtkProject::project` and `LdtkLevel::level` fields have both been
renamed to `data`
([#206](#206))
* All fields of `LdtkProject` and `LdtkLevel` are now privatized, and
have immutable getter methods
([#206](#206))
* `LevelMap` and `TilesetMap` type aliases have been removed
([#206](#206))
* `LdtkAsset` and `LdtkProject` are now exported in new `assets` module
instead of `lib.rs`
([#206](#206))
* asset `Loader` types are now private
([#206](#206))
* `LdtkAsset` renamed to `LdtkProject`
([#206](#206))

### Features

* add `LevelIndices` type defining a level's location in a project and
use it in `LevelSelection::Indices`
([#221](#221))
([59618fe](59618fe))
* add `RawLevelAccessor` trait for `LdtkJson` level borrowing/iteration,
replacing existing methods
([#225](#225))
([d3de2d9](d3de2d9))
* add `SpawnExclusions` to `LdtkSettings` for skipping layers by
identifier ([#275](#275))
([282404d](282404d)),
closes [#272](#272)
* add layer entity for Entity layers, changing the hierarchy
([#257](#257))
([ee20a53](ee20a53))
* add LdtkJsonWithMetadata type for representing internal- and
external-level project data with generics
([#242](#242))
([630434a](630434a))
* add LdtkProjectData for representing either internal- or
external-level project data concretely
([#243](#243))
([c530bc9](c530bc9))
* add level locale types and begin splitting internal_levels and
external_levels features
([#237](#237))
([8129e55](8129e55))
* add LevelIid component and spawn it on every level
([#215](#215))
([ad83455](ad83455))
* add LoadedLevel type that wraps around levels with complete data
([#214](#214))
([3d40c15](3d40c15))
* add types and traits around LevelMetadata
([#229](#229))
([382dea2](382dea2))
* change `LevelEvent` inner types from `String` to `LevelIid`
([#219](#219))
([0039ed7](0039ed7))
* change `LevelSelection::Iid` inner type from `String` to `LevelIid`
([#219](#219))
([0039ed7](0039ed7))
* change `LevelSet` inner type from `String` to `LevelIid`
([#219](#219))
([0039ed7](0039ed7))
* LdtkLevel renamed to LdtkExternalLevel and is no longer used as a
component ([#244](#244))
([670cd4e](670cd4e))
* redesign LdtkProject with better level data accessors and correct
modeling of internal/external levels
([#244](#244))
([670cd4e](670cd4e))
* replace `LevelSet::from_iid` with `LevelSet::from_iids`, which can
convert from any collection of strings.
([#219](#219))
([0039ed7](0039ed7))
* upgrade to bevy 0.12
([#265](#265))
([194731e](194731e))
* upgrade to LDtk 1.5.3, dropping support for previous versions
([#295](#295))
([4926a50](4926a50))
* upgrade to LDtk types and examples to 1.4.1 (drop support for
&lt;1.4.1) ([#256](#256))
([ab21e2c](ab21e2c))
* use new LevelIid type in LevelEvent, LevelSet, and LevelSelection,
plus other improvements
([#219](#219))
([0039ed7](0039ed7))
* use the bundle's `Default` implementation rather than the field's in
`LdtkEntity` and `LdtkIntCell` derive macros
([#222](#222))
([f003127](f003127))


### Bug Fixes

* don't apply level set until project and dependencies are completely
loaded ([#296](#296))
([dbfe1c6](dbfe1c6))
* normalize resolved asset paths using `path_clean`
([#255](#255))
([33a8998](33a8998)),
closes [#240](#240)
* only spawn invisible tiles on first sub-layer of AutoTile+IntGrid
layers ([#231](#231))
([d2873e3](d2873e3))
* recalculate layer offset to adjust for tileset sizes
([#254](#254))
([c00085d](c00085d))
* use entity definition tile size instead of entity instance tile size
as basis when calculating ldtk entity scale
([#271](#271))
([833af01](833af01))


### Documentation Changes

* add 0.8 to 0.9 migration guide
([#266](#266))
([bb91660](bb91660))
* add collectathon cargo example
([#288](#288))
([32dfb85](32dfb85))
* add mdbook with outline and introduction
([#261](#261))
([810b25a](810b25a))
* add tile-based game example w/ a tutorial in the book, replacing
getting-started guide
([#269](#269))
([2d43efa](2d43efa))
* document all-features in docs.rs
([#252](#252))
([321bb07](321bb07))
* reference book in API ref and README.md, replacing redundant sections
([#282](#282))
([e7afdad](e7afdad))
* remove README.md caveat for hot reloading external levels
([#253](#253))
([59eb6b3](59eb6b3))
* write *Anatomy of the World* chapter of book
([#285](#285))
([29d5e33](29d5e33))
* write *Create bevy relations from ldtk entity references* chapter of
book ([#287](#287))
([8080f24](8080f24))
* write *Game Logic Integration* chapter of the book
([#279](#279))
([a62a556](a62a556))
* write *Level Selection* chapter of book
([#284](#284))
([226c60c](226c60c))
* write *Make level selection follow player* chapter of book
([#293](#293))
([201d908](201d908))
* write *Respawn levels and worlds* chapter of book
([#289](#289))
([55ed30f](55ed30f))


### Code Refactors

* `LdtkAsset` and `LdtkProject` are now exported in new `assets` module
instead of `lib.rs`
([#206](#206))
([fe44774](fe44774))
* `LdtkAsset` renamed to `LdtkProject`
([#206](#206))
([fe44774](fe44774))
* `LdtkProject::project` and `LdtkLevel::level` fields have both been
renamed to `data`
([#206](#206))
([fe44774](fe44774))
* `LevelMap` and `TilesetMap` type aliases have been removed
([#206](#206))
([fe44774](fe44774))
* All fields of `LdtkProject` and `LdtkLevel` are now privatized, and
have immutable getter methods
([#206](#206))
([fe44774](fe44774))
* asset `Loader` types are now private
([#206](#206))
([fe44774](fe44774))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Trevor Lovell <[email protected]>
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.

[feat] Add an option to disable spawning of some/all IntGrid entities
2 participants