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

Remove Data types from the interface #163

Merged
merged 12 commits into from
Feb 16, 2022
Merged

Conversation

aleokdev
Copy link
Contributor

Closes #162 & closes #158.

@aleokdev aleokdev added enhancement breaking This change breaks backwards-compatibility labels Feb 16, 2022
@aleokdev
Copy link
Contributor Author

aleokdev commented Feb 16, 2022

I'd love to use a proc macro instead of map_wrapper so we wouldn't have duplicate docs in data members + interface properties but both syn and quote are too heavy to use.

@aleokdev
Copy link
Contributor Author

Tile::collision is not implemented yet. Having an object group there is giving a few issues.

@aleokdev
Copy link
Contributor Author

Had to end up making both ObjectLayerData and ObjectData public. There isn't really any issue with that and it's probably the type collisions should use (It's raw object data, not belonging to any map). However it's inconsistent with everything else.

@bjorn
Copy link
Member

bjorn commented Feb 16, 2022

There isn't really any issue with that and it's probably the type collisions should use (It's raw object data, not belonging to any map). However it's inconsistent with everything else.

Yeah, the inconsistency isn't very nice, and in fact I've been struggling with the same in Tiled, for example to avoid placing tile objects in these object groups and workarounds for assigning IDs to these objects, which are normally counted per map. But I guess we can live with it.

bjorn
bjorn previously approved these changes Feb 16, 2022
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Great work!

map,
group,
index: 0,
}
Copy link
Member

Choose a reason for hiding this comment

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

What is the logic behind these reformattings? Here it's wrapping them all on their own line, but just a few lines up you removed some wrapping to form Ok((Self { layers }, properties)). I could be fine with either notation, but why keep changing these around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason for it, just rustfmt doing its thing.

Copy link
Member

Choose a reason for hiding this comment

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

Can we do a single pass of rustfmt to avoid these changes, or does it actually like to change the wrapping back and forth?

@aleokdev aleokdev merged commit 4227705 into mapeditor:master Feb 16, 2022
@aleokdev aleokdev deleted the internal-data branch February 16, 2022 15:26
aleokdev added a commit to aleokdev/rs-tiled that referenced this pull request Feb 23, 2022
* Add properties to `Object`

* Add properties to `FiniteTileLayer`

* Add properties to `Layer`

* Add properties to `ImageLayer`

* Add properties to `ObjectLayerData`

* Remove `data()`; Replace `map()`

* Add properties to `LayerTile`

* `MapWrapper` -> `map_wrapper!`

* Fix examples/tests

* Limit visibility of data types

* Make `Tile::collision` public again

* Doc tweak

Co-authored-by: Thorbjørn Lindeijer <bjorn@lindeijer.nl>
@aleokdev aleokdev added this to the 0.10.0 milestone Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change breaks backwards-compatibility enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Data types from the interface
2 participants