mapeditor / rs-tiled

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

Add `as_x` fns for layer types #235

Closed aleokdev closed 1 year ago

aleokdev commented 2 years ago

Improves crate ergonomics. Most of the time when parsing you are only interested in one layer type at a time so it's much nicer to use layer.layer_type().as_tiles() than the whole match block. Specially nice for closures in filters:

// Old
let tile_layers = self
    .map
    .layers()
    .filter_map(|l|
        match l.layer_type() {
            tiled::LayerType::Tiles(tl) => Some((l, tl)),
            _ => None,
        }
    );

// New
let tile_layers = self
    .map
    .layers()
    .filter_map(|l| Some((l, l.layer_type().as_tiles()?)));

Crates like serde_json do this with some of their enums.

bjorn commented 2 years ago

That seems useful indeed. One thing I'm wondering, since it seems that in all cases you'd need to call .layer_type() first, would it make sense to put the as_tiles, etc. functions on Layer instead of LayerType, similar to the functions that used to be in the tests? And maybe then using those names instead, so they match their return value?

aleokdev commented 2 years ago

Not 100% sure. For symmetry wouldn't that mean we should also have those methods in LayerType as well?

bjorn commented 2 years ago

Not 100% sure. For symmetry wouldn't that mean we should also have those methods in LayerType as well?

As a pure convenience addition, I think that depends on whether it is a common scenario to have a LayerType instance without having access to the related Layer, but I have my doubts about that.

In my mind, the "LayerType" enum is a kind of workaround for the lack of inheritance in Rust, so I welcome these functions as a way to let this type fade a bit more into the background. We keep layer_type() for those cases where it is really more convenient to do a match, but I don't see a need to provide the as_x convenience functions also there.

deifactor commented 1 year ago

I've only used this crate in one project, but IMO I don't see any use for having a LayerType without a Layer; I agree that it feels very much like an 'implementation detail'. So I'd just define the methods directly on Layer itself (as well as as_finite_tiles and as_infinite_tiles). If there does wind up being a need for putting it on the LayerType then you can just move the methods to LayerType and delegate.

In terms of naming, I like as_tile_layer, as_object_layer and so on. as_tiles makes me think it'll give me a Vec<Tile> or something.

aleokdev commented 1 year ago

Oops, this took so long to merge that the changelog ended up in the wrong place. Will need to fix that at some point.