Adding tileset index

[Anti-Alias]

I wish to include the tileset index in the LayerTile<'map> facade.
This would assist in making simple rendering code significantly faster.

Say I've loaded my map file and acquire the tilesets:

let map = Map::parse_xml(...);
let tilesets = map.tilesets();

Now, say I load all of the textures of the tilesets.
Assume they're all single-image tilesets.
textures is parallel to tilesets

let textures = tilesets.map(|tileset| load_texture(tileset.image.unwrap().source));

Now, say my rendering code looks something like this:

fn render(map: &Map, textures: &[Texture], batch: &mut Batch) {
    let tw = map.tile_width as f32;
    let th = map.tile_height as f32;
    for layer in map.layers() {

        // Assume all layers are tile layers for simplicity
        let layer = match layer.layer_type() {
            LayerDataType::TileLayer(data) => data,
            _ => panic!("Only tile layers supported")
        }
    
        // For all tiles in the layer...
        for x in 0..map.width {
            for y in 0..map.height {
                let layer_tile = layer.get_tile(x, y);
                if let Some(tile) = tile {

                    // Including a `tileset_index` field in LayerTile<'map> would make the rendering code MUCH more efficient in this example.
                    // The current alternative is to make `textures` a HashMap<String, Texture> where String is the tileset name.
                    // For every LayerTile, the Tileset it belongs to would need to be "looked up" by String.
                    //
                    // Admittedly, this could be mitigated by making the graphics structure something like:
                    //     let mut graphics = HashMap<usize, Mesh>::new();
                    // Where the usize is the texture index and the Mesh is just the polyon mesh which uses that texture.
                    //
                    // However, when building such a structure, there would still need to be a hash lookup per tile.
                    // In this scenario, it would hurt the loading time for a game, though not the overall performance.
                    let texture = &textures[layer_tile.tileset_index];

                    // I think I also heard that there was talk of adding UVs to the tileset tiles themselves.
                    // That would definitely make this operation faster as well.
                    let (uv1, uv2) = get_tile_uvs(tile.id, &*map.tilesets[layer_tile.tileset_index]);

                    // Draws tile
                    batch.draw(
                        &texture,
                        uv1, uv2,
                        x as f32, y as f32,
                        tw, th
                    );
                }
            }
        }
    }
}

Having a tileset index in the LayerTile<'map> facade type would make rendering the map to a batch much faster.
This unfortunately exposes the tileset's "id" which I am guessing we'd rather not do unless we have to.
I think that rendering code like this is common enough to warrant this sort of change.
Let me know what you all think, though.
There may already be an efficient way of doing this with the current codebase I'm not aware of, and I've just got tunnel vision.

[Anti-Alias]

Another way of doing this would be to store the index in the tileset itself.
That would make the facade type LayerTile<'map> slightly cheaper to construct, though makes Tileset more tightly coupled with Map, which might be awkward.

This is my preferred way at the moment.

[bjorn]

Right, it's not possible to store the tileset index on the tileset, because a tileset can be used by multiple loaded maps, and it could have a different index in each of those maps.

[aleokdev]

This makes it obvious that we really need to change these public members for properties. Other than that, maybe we should store a reference to the map rather than the tileset itself in the tile, and then have a tileset property that indexes into the tilesets of the map.

[bjorn]

Could it be that we went too far with our desire to make things "easy" for the user, when introducing LayerTile<'map>? Maybe we should just stick with LayerTileData (and maybe rename it to TileInstance or TileRef or something). Because indeed once you have the tileset_index, getting a &Tileset is already trivial and you might not even need it, as this example shows.

The get_tile helper function could just be moved to Map.

[aleokdev]

If the issue is that the user might not need the tileset, then we can just convert LayerTile into a MapWrapper<'map, TileLayerData> and add a tileset property that just does self.map.tiledets()[self.tileset_index].

[aleokdev]

Could it be that we went too far with our desire to make things "easy" for the user, when introducing LayerTile<'map>? Maybe we should just stick with LayerTileData (and maybe rename it to TileInstance or TileRef or something). Because indeed once you have the tileset_index, getting a &Tileset is already trivial and you might not even need it, as this example shows.

The get_tile helper function could just be moved to Map.

The whole ordeal about wrapping everything with a map/tileset was to save the user from having to do exactly this (i.e. Having to do their own wrapper over a tile+map if they want to pass the tile somewhere else). If getting a tileset is trivial, then we should just bundle the map with the tile, not the tileset.

[bjorn]

If the issue is that the user might not need the tileset, then we can just convert LayerTile into a MapWrapper<'map, TileLayerData> and add a tileset property that just does self.map.tiledets()[self.tileset_index].

Ah, that sounds like a clean solution indeed!

[aleokdev]

#158 will fix this. I'll try to submit a PR ASAP. For the time being, this can be a workaround.