New renderer #62
New renderer #62
Conversation
There was a problem hiding this comment.
Really cool prototype, I haven't tried it out yet, but I will try to in coming days. What are your thoughts on the performance impact of having an entity per tile? Does bevy_ecs_tilemap reuse meshes for two layer tiles that are the same tile? That could be really awesome for adding in support for spawning multiple copies of a layer/map. If it's something already supported, do you think we can abstract that for use with objects too?
Before integration it would be good to have some clarity on that and I think we also need to validate web, as I know that's very important to a lot of us ( though I haven't gotten our game running on web yet...). One of the next steps I was going to undertake was to have a better way of indexing/querying tiles.. If we're actually just generating an entity per tile, that will make it exceptionally easier to find the data we want, so we should think about whether we can make it more queryable so we could --for example--write a system to spawn a tilemap layer or object group that would query the tiles it needs without needing to access the Asset again.
| ) { | ||
| // quick and dirty, run this for all textures every time a map is created/modified | ||
| for event in texture_events.iter() { | ||
| match event { | ||
| AssetEvent::Created { handle } => { | ||
| if let Some(mut texture) = textures.get_mut(handle){ | ||
| texture.sampler.min_filter = FilterMode::Nearest; | ||
| } |
There was a problem hiding this comment.
If we're going through all the trouble to do this in every example, should we consider enabling it as a feature or with a boolean flag in a config object?
There was a problem hiding this comment.
We could have a feature that queries textures for the tiled map layers and updates them to use min_filter. I think having the user do this during a "loading" game state is probably better though.
| if let Some(tile) = tileset.tiles.iter().find(|tile| tile.id == tile_id) { | ||
| if let Some(animations) = tile.animation.clone() { | ||
| let animation = Animation { | ||
| frames: animations.iter().map(|frame| Frame { | ||
| tile_id: frame.tile_id, | ||
| duration: (frame.duration as f64) / 1000.0, | ||
| }).collect(), | ||
| current_frame: 0, | ||
| last_update: 0.0, | ||
| }; |
There was a problem hiding this comment.
Is this specifically for tile layer animations? I didn't even know that was possible... I was mainly indicating we should have animation support in objects. It shouldn't be hard to adapt it for that purpose. We should probably separately process tiles and store them before we make tile layers or objects that would both clone from the same tile struct.
There was a problem hiding this comment.
This is for per tile animations. It's pretty cool how it works and we likely can use the same thing for objects.
| let world = app.world_mut(); | ||
| add_tile_map_graph(world); | ||
| .add_system(process_loaded_tile_maps.system()) | ||
| .add_system(animation::update.system()); |
There was a problem hiding this comment.
once we start doing this, we probably want to setup animations (And auto-spawn) as plugins for the library consumer to invoke
There was a problem hiding this comment.
Yeah it might be worth separating out the animation stuff into its own plugin.
| @@ -37,7 +26,7 @@ impl AssetLoader for TiledMapLoader { | |||
| ) -> BoxedFuture<'a, Result<(), anyhow::Error>> { | |||
| Box::pin(async move { | |||
| let path = load_context.path(); | |||
| let mut map = Map::try_from_bytes(self.asset_folder.as_path(), path, bytes.into())?; | |||
| let mut map = TiledMap::try_from_bytes(self.asset_folder.as_path(), path, bytes.into())?; | |||
There was a problem hiding this comment.
Happy to see us continuing to move along the path of making the naming unique versus rs-tiled structs --- I just didn't want to break compatibility but happy to do this as a 0.3.0 release thing
There was a problem hiding this comment.
Yeah I want to essentially even drop the loaded tiled object from memory once we've generated the map entities, however I don't think we are quite there yet in terms of what we can support(objects, etc).
| @@ -0,0 +1,196 @@ | |||
| use anyhow::Result; | |||
There was a problem hiding this comment.
so FWIW ... it's kind-of hard to see what's been updated in this code because the renaming of the file and the changes happened in the same commit.
There was a problem hiding this comment.
Sorry, I wasn't super happy with all of the older code a lot of it got deleted. :p
| Use `default-features=false, features=["web"]` in your project's `Cargo.toml`. Tiled maps using Zstd compression are not supported. | ||
|
|
||
| ## Top-needed features | ||
|
|
||
| * better support for isometric maps | ||
| * support for embeded objects in tiles | ||
| * support for embedded images in Tmx files | ||
| * support for animations | ||
| * ~~support for animations~~ done |
| pub last_update: f64, | ||
| } | ||
|
|
||
| pub fn update( |
| project_ortho(chunk_pos, tile_width, tile_height); | ||
| let mut map = Map::new( | ||
| Vec2::new((tiled_map.width as f32 / 64.0).ceil(), (tiled_map.height as f32 / 64.0).ceil()).into(), | ||
| Vec2::new(64.0, 64.0).into(), |
There was a problem hiding this comment.
Yes, this is the chunk size, its relatively unimportant, but should be configurable by the user.
| for x in 0..tiled_map.width as usize { | ||
| for y in 0..tiled_map.height as usize { | ||
| let map_tile = match &layer.tiles { | ||
| tiled::LayerData::Finite(tiles) => &tiles[y][x], |
There was a problem hiding this comment.
Not blocking for this PR, but FYI row/column layer data is a bit dicey at the moment, see #48
| let mut map_entity = None; | ||
| commands.entity(parent_entity).with_children(|child_builder| { | ||
| map_entity = Some(child_builder.spawn().id()); | ||
| }); | ||
| let map_entity = map_entity.unwrap(); |
There was a problem hiding this comment.
One thing we may want to consider down the line is how we would spawn multiple versions of the same tile map layer. Autoreload is super helpful during development, but we were limited in improving this API by the fact that there was no top-level entity. I like the potential we've added with the new structure.
Also, this construction with the unwrap() seems odd to me, though I see why you do it. If this is the only option, maybe we can change to expect() so that we can easily see what went wrong if there's a panic?
There was a problem hiding this comment.
In bevy_ecs_tilemap I would like to have the current Map entity be a layer and then create a new Map entity which has layer children. The hierarchy would look like:
Map > Layer > Chunk > Tile
|
|
WIP