search

mapeditor / rs-tiled

Reads files from the Tiled editor into Rust
https://crates.io/crates/tiled
MIT License
268 stars 103 forks source link

More property/member consistency #168

Closed aleokdev closed 2 years ago

aleokdev commented 2 years ago

Right now, there are some members that shouldn't be public, as they should be protected against mutation; e.g. Map::width/height/tile_width. These should be changed to properties.

bjorn commented 2 years ago

Just wondering, but why should these be protected against mutation? (I think the change makes sense for consistency, regardless)

aleokdev commented 2 years ago

Well, although right now these properties aren't used internally (as far as i know), we should protect them in case that we rely on them later on for anything. And please, do not worry about writing to TMX files right now. When we do implement writing (if we do) then we can add set_ functions as well.

My thinking right now for when to use properties vs when to use members is how they are being used internally. If the values are only useful from the user side and have no relevance within the crate (e.g. tint color, opacity, etc) these could be exposed as public members. In the case that they do have relevance within the crate and may cause invalid behavior if modified (e.g. width/height on a tile layer) OR we have some members that could be exposed but can't because they're contained within a private structure (e.g. internal data types) then we should use properties as well.

(edit) Maybe tile_x can stay as exposed members, as I doubt we'll rely on them internally.

aleokdev commented 2 years ago

One thing though; I still don't understand why layers must have a width/height attribute if the map already has them. Is it because of backwards compatibility?

bjorn commented 2 years ago

One thing though; I still don't understand why layers must have a width/height attribute if the map already has them. Is it because of backwards compatibility?

Yes, layer size does not have to match the size of the map due to backwards compatibility. This goes way back to Tiled Java though, but I've kept this because I was thinking to possibly support this as a feature again in future versions of Tiled.

Eventually I'd also like to support a different tile size and orientation per layer, but I'm not sure how to support this yet. It might be quite a refactoring. Or, it could be done by introducing another top-level container for multiple maps.