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.

Sign up for GitHub

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

Jump to bottom

feat!: More intuitive default instantiation behavior. #222

Merged
merged 7 commits into from
Sep 18, 2023
Merged

feat!: More intuitive default instantiation behavior. #222

merged 7 commits into from
Sep 18, 2023

Conversation

elenakrittik
Copy link
Contributor

@elenakrittik elenakrittik commented Aug 19, 2023 •
edited
Loading

Changes the default behavior to use bundle's struct's Default impl instead of each individual field's.

Solves #119.

  • I have properly formatted code using cargo fmt.
  • I have tested the code and documentation i changed by both crafting and running an example and by running cargo test.
  • I have ran cargo clippy and fixed any issues arising from my changes.

@elenakrittik elenakrittik marked this pull request as ready for review August 19, 2023 17:06
Trouv
Trouv reviewed Aug 19, 2023
View reviewed changes
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.

It may be better to just make this the normal behavior, not opt-in or opt-out. It's certainly more flexible to make it opt-in like this, but I think the current behavior is a little confusing and it would be fine to just get rid of it. Making this the only behavior would also be a bit easier to maintain/test in the future, rather than having one more toggle-like feature to worry about. Also, I've been considering actually scaling back the macro functionality in future releases (not soon, like 0.11 or something), and if that comes to pass the opt-inness of this would probably be on the chopping block anyway.

Down side would be that it's a breaking change, but I'm not too worried about that. This library is far from stable, and I'm planning to release 0.9 with many breaking changes with plenty of time before bevy 0.12 comes out so people can upgrade at their own pace. Thoughts?

@elenakrittik
Copy link
Contributor Author

elenakrittik commented Aug 19, 2023 •
edited
Loading

I personally totally share your vision and if breaking changes are for the better, then i'll make them. But at the same time i recognize that there's a plenty of developers with busy schedules and that they would rather prefer to receive bug fixes without the accompanying breaking changes. Though, in case of Bevy's ecosystem, i imagine most people will be fine with breaking changes, or pin bevy-related dependencies to specific versions and upgrade only when they're ready. I'll change this PR a bit later.

@elenakrittik
Copy link
Contributor Author

Done. Due to removal of backwards compatibility had to add lots of #[derive(Default)]s.

@elenakrittik elenakrittik changed the title feat: #[bevy_ecs_ldtk(use_default_impl)] feat!: #[bevy_ecs_ldtk(use_default_impl)] Aug 19, 2023
@elenakrittik elenakrittik changed the title feat!: #[bevy_ecs_ldtk(use_default_impl)] feat!: More intuitive default instantiation behavior. Aug 19, 2023
Trouv
Trouv requested changes Aug 20, 2023
View reviewed changes
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.

Thanks for making the breaking change, I think it's a bit better this way

macros/src/ldtk_entity.rs Show resolved Hide resolved
examples/basic.rs Show resolved Hide resolved
src/app/ldtk_entity.rs Show resolved Hide resolved
src/app/ldtk_int_cell.rs Show resolved Hide resolved
@elenakrittik
Copy link
Contributor Author

Done, sorry for this simple change taking so long.

Trouv
Trouv approved these changes Sep 18, 2023
View reviewed changes
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.

Done, sorry for this simple change taking so long.

No worries. We're all volunteers here. Thanks again for the contribution!

@Trouv Trouv merged commit f003127 into Trouv:main Sep 18, 2023
4 checks passed
@Trouv Trouv mentioned this pull request Sep 18, 2023
Closed
@github-actions github-actions bot mentioned this pull request Sep 18, 2023
Merged
@elenakrittik elenakrittik deleted the feat/use-default-impl branch September 18, 2023 04:03
Trouv added a commit that referenced this pull request Feb 11, 2024
@github-actions @Trouv
chore(main): release 0.9.0 (#213)
e040747 
🤖 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 <trevorlovelldesign@gmail.com>
@neocturne neocturne mentioned this pull request Mar 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trouv Trouv Trouv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@elenakrittik @Trouv