Upgrade tiled dep and add support for image-per-tile tilesets. #381

dvogel · 2023-01-01T17:12:32Z

This does two things:

Adds support for tilesets that use a separate image per tile.
Upgrades the tiled dependency. Should be no change in actual functionality but the 0.10 version is much easier to work with than the 0.9 version. (fixes Update tiled to version 0.10 #249 and Newer tiled dependency planned? #320).

Here is a screenshot of the example running with this code:

Here is a screenshot of a map loaded with the new image-per-tile support (this file has 5 layers and each is rendered as I see it in Tiled):

examples/helpers/tiled.rs

rparrett · 2023-01-06T03:11:08Z

This doesn't appear to work with my map. It was saved with Tiled 1.4, so I think it should be compatible with rs-tiled 0.10.

https://github.com/rparrett/taipo/blob/main/assets/textures/level1.tmx

rparrett · 2023-01-06T03:59:09Z

Actually, this isn't working properly with the in-repo example either. It should look like this:

dvogel · 2023-01-06T04:20:44Z

Thanks for reviewing this @rparrett. I'm not sure why I didn't notice the obvious error for the test map rendering. I'll take another pass at this soon.

dvogel · 2023-01-06T04:49:42Z

I just pushed a change that incorporates your suggestions and fixes the test map loading. Here's screenshots of the test map being rendered as it is by default and then zoomed out to see the entire thing.

The spacing between tiles looks a little strange but I think it is a rendering artifact rather than a tile placement error because it goes away when I zoom in. Would be good to test on another machine though.

I wasn't able to test with your map @rparrett because it doesn't include the tileset image.

rparrett

Looking good on my end now. (tileset image for level1.tmx is in the same repo)

I'd still like to take a slightly closer look when I have a few more minutes, especially with a map with multiple "single image" tilesets and layers.

I wouldn't expect the visual artifacts you mentioned to be related to this PR.

rparrett · 2023-01-06T05:18:57Z

examples/helpers/tiled.rs

-                            x: tiled_map.map.tile_width as f32,
-                            y: tiled_map.map.tile_height as f32,
-                        };
+                        match tile_layer {


Can we collapse this match into a let else guard also?

rparrett · 2023-01-06T05:24:27Z

examples/helpers/tiled.rs

-                            y: tiled_map.map.height,
+                        let tiled::LayerType::TileLayer(tile_layer) = layer.layer_type() else {
+                            log::info!(
+                                "Skipping layer because {:?} is not supported.",


It seems like the debug impl for LayerType prints all the map data somehow, making this spew several screens of spam for my map.

Maybe something like

log::info!( "Skipping layer {} because it is an unsupported type", layer.id() );

would be adequate?

rparrett · 2023-01-06T05:26:49Z

examples/helpers/tiled.rs

+                                    .insert(layer_index as u32, layer_entity);
+                            }
+                            _ => {
+                                log::info!(


Same thing here about the debug print.

rparrett · 2023-01-06T20:35:09Z

examples/helpers/tiled.rs

-                                    })
-                                    .id();
-                                tile_storage.set(&tile_pos, tile_entity);
+                                let texture = if tiled_map.tile_texture_indexes.is_empty() {


It seems like it should be possible to allow "image collection tilesets" as well as normal tilesets in a single map, provided each layer exclusively uses one or the other type of tileset.

But this code causes TilemapTexture::Vector to be used for every layer if any layer uses an image collection tileset.

rparrett · 2023-01-06T20:57:57Z

A couple more notes:

This previously (prior to this PR) worked with grouped layers, but now any layer in a group is skipped.

This seems like sort of a pain to deal with properly (groups may be nested). This is just an example, so maybe we shouldn't worry about that.
cargo run --example tiled --features=atlas fails to compile due to the use of TilemapTexture::Vector.

I guess we need to gate some of this code on the atlas feature, and leave "image collection" tilesets unsupported when that feature is enabled.

dvogel · 2023-01-07T19:58:48Z

I just pushed a commit that unifies the handling of single image and image collection tilesets, skipping the latter when the atlas feature is enabled. I also updated map.tmx to use these features.

Here is a screenshot with the atlas feature enabled and another with it disabled:

In the second screenshot you can see the castles because they use an image collection tileset.

Here's is the rendering of the level1.tmx file you linked to earlier:

I haven't had a chance to test a layer that uses multiple tilesets yet. I plan to do that soon but I've run out of time for the day.

dvogel · 2023-01-07T20:05:40Z

examples/helpers/tiled.rs

+                            TilemapTexture::Single(tile_images[0].clone_weak())
+                        } else {
+                            #[cfg(feature = "atlas")]
+                            panic!("Cannot use Tiled maps with a tileset that uses a collection of images due to incompatibility with the 'atlas' feature.");


This should not really panic because the tileset should be skipped earlier in the loop. However I wasn't sure how to satisfy the compiler without an even more extreme solution, such as generating a placeholder TilemapTexture::Single() here.

I had an idea for cleaning up this and related code: rparrett@b9e707c

Unfortunately, this turned out to be a bad idea, as it also can't compile with the atlas feature. But maybe some parts are still useful.

Fixed that up in a new commit here: rparrett@b794f2d which seems to be behaving, apart from some warnings that should probably be cleaned up when compiled with atlas

That looks good to me. Works in my game. I like the idea of creating the Single and Vector structs earlier in the loading process. I had tried to do that but ran into some ownership issues and dialed it back for time.

rparrett

There are a couple warnings being generated still when building with the atlas feature. I couldn't figure out a way to do that any more elegantly than sprinkling more #[cfg(not(feature = "atlas"))] where rust is complaining.

examples/helpers/tiled.rs

bzm3r

@dvogel @rparrett this is generally looking good to me. Feel free to ping me once you've settled down on solutions overall/CI is passing, and I'll merge it!

(currently, CI is not failing, but it has some review suggestions, some unnecessary muts apparently?)

(thanks very much for your contributions!)

Co-authored-by: Rob Parrett <robparrett@gmail.com>

dvogel · 2023-01-11T05:03:58Z

I was able to suppress all of the remaining cargo check errors by completely removing the tile_image_offsets member from TiledMap when the atlas feature is enabled.

rparrett

This seems to be working well on my end now. Thanks for all the effort to make this work with the atlas feature.

It's a bit unfortunate that this code that is likely to end up copy/pasted into users' projects won't work in those projects as-is. (due to feature gating on a bevy_ecs_tilemap feature). Users would need to manually fix up the feature gated code based on whether or not they need the atlas feature.

That seems preferable to other options

not supporting image collection tilesets
adding a completely separate TiledMapPlugin helper and feature gate in examples/tiled.rs instead
?

Perhaps we should at least add a comment at the top somewhere explaining the situation.

bzm3r · 2023-01-11T05:35:52Z

* adding a completely separate `TiledMapPlugin` helper and feature gate in `examples/tiled.rs` instead

Hmm, I am thinking: this might be a pretty good idea. Speaking from an end-user perspective, it seems clearer?

But we can leave this for another PR. For now, we can merge as is, with a comment explaining the situation.

bzm3r

This looks good to me overall. We can further refine it new PRs. I do like @rparrett 's suggestion that we should leave a comment explaining the situation, so that's the only change I am going to request!

dvogel · 2023-01-13T03:43:25Z

I just pushed a comment with an explanation of how the code will need to be cleaned up when adopted. @rparrett please double-check my explanation as it took me a few reads through your comments to understand that you meant their code wouldn't compile once copied over.

Hmm, I am thinking: this might be a pretty good idea. Speaking from an end-user perspective, it seems clearer?

To be honest I was a initially a little lost re: this feature. I didn't realize it was an option until well into trying to update this helper. I think even with the comment I added to the top of the helper a new user may struggle to understand what to do since the atlas feature isn't advertised up front. As best I can tell it is an optimization to load sprite sheets into the GPU more efficiently than could be done otherwise. I don't understand how it is incompatible with the alternative tilemap texture modes though. My current impression is that the feature could be removed without much downside. I assume that's not actually true, but whatever I'm missing is likely a good piece to add to the documentation :)

Long term it seems like maybe this could become full-featured enough to remain in the bevy_ecs_tilemap crate, re-exported for end-users to use in their projects. That would bring the compile time choices along with it.

rparrett · 2023-01-13T04:04:32Z

As best I can tell it is an optimization to load sprite sheets into the GPU more efficiently than could be done otherwise. I don't understand how it is incompatible with the alternative tilemap texture modes though. My current impression is that the feature could be removed without much downside. I assume that's not actually true, but whatever I'm missing is likely a good piece to add to the documentation :)

The feature is not well documented. I left a comment in there that might be helpful for understanding the atlas feature. But the gist is that it's there for compatibility with webgl2.

their code wouldn't compile once copied over.

Right, yeah. It's unfortunate that I didn't realize that sooner. I think the comment generally looks good.

Although now that it's all spelled out, it feels quite bad to ask users to do all that. Looking forward to when the atlas feature is eventually removed, I am liking the idea of splitting the file more and more.

We should definitely follow up on that before the next release if there's consensus about that.

bzm3r · 2023-01-13T05:08:35Z

Thank you so much, dvogel and rparrett. 🙏

It will be nice indeed when we can yeet atlas....

Upgrade tiled dep and add support for image-per-tile tilesets.

0973a25

rparrett reviewed Jan 6, 2023

View reviewed changes

examples/helpers/tiled.rs Outdated Show resolved Hide resolved

rparrett reviewed Jan 6, 2023

View reviewed changes

examples/helpers/tiled.rs Outdated Show resolved Hide resolved

Fix loading of single-image tilesets.

70c5c99

dvogel requested a review from rparrett January 6, 2023 04:51

rparrett reviewed Jan 6, 2023

View reviewed changes

Support Tiled maps with both types of tilesets.

97d3702

Avoid match indentation.

03265d1

dvogel commented Jan 7, 2023

View reviewed changes

Drew Vogel and others added 2 commits January 7, 2023 14:07

Drop unnecessary Debug derivation.

a656264

Build TilemapTexture during asset loading

267b73c

rparrett reviewed Jan 9, 2023

View reviewed changes

examples/helpers/tiled.rs Outdated Show resolved Hide resolved

bzm3r reviewed Jan 10, 2023

View reviewed changes

dvogel and others added 2 commits January 10, 2023 21:55

Update examples/helpers/tiled.rs

2a83075

Co-authored-by: Rob Parrett <robparrett@gmail.com>

Avoid all cargo check complaints re: the atlas feature.

4d5e7ca

dvogel requested a review from bzm3r January 11, 2023 05:02

rparrett approved these changes Jan 11, 2023

View reviewed changes

bzm3r requested changes Jan 11, 2023

View reviewed changes

Explain how to use helpers/tiled.rs

d4e0e64

dvogel requested a review from bzm3r January 13, 2023 03:43

bzm3r approved these changes Jan 13, 2023

View reviewed changes

bzm3r merged commit c2c3659 into StarArawn:main Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade tiled dep and add support for image-per-tile tilesets. #381

Upgrade tiled dep and add support for image-per-tile tilesets. #381

dvogel commented Jan 1, 2023

rparrett commented Jan 6, 2023 •

edited

Loading

rparrett commented Jan 6, 2023

dvogel commented Jan 6, 2023

dvogel commented Jan 6, 2023

rparrett left a comment

rparrett Jan 6, 2023

rparrett Jan 6, 2023

rparrett Jan 6, 2023

rparrett Jan 6, 2023

rparrett commented Jan 6, 2023

dvogel commented Jan 7, 2023

dvogel Jan 7, 2023

rparrett Jan 8, 2023

rparrett Jan 8, 2023 •

edited

Loading

rparrett Jan 8, 2023 •

edited

Loading

dvogel Jan 8, 2023

rparrett left a comment

bzm3r left a comment •

edited

Loading

dvogel commented Jan 11, 2023

rparrett left a comment •

edited

Loading

bzm3r commented Jan 11, 2023

bzm3r left a comment

dvogel commented Jan 13, 2023

rparrett commented Jan 13, 2023

bzm3r commented Jan 13, 2023

Upgrade tiled dep and add support for image-per-tile tilesets. #381

Upgrade tiled dep and add support for image-per-tile tilesets. #381

Conversation

dvogel commented Jan 1, 2023

rparrett commented Jan 6, 2023 • edited Loading

rparrett commented Jan 6, 2023

dvogel commented Jan 6, 2023

dvogel commented Jan 6, 2023

rparrett left a comment

Choose a reason for hiding this comment

rparrett Jan 6, 2023

Choose a reason for hiding this comment

rparrett Jan 6, 2023

Choose a reason for hiding this comment

rparrett Jan 6, 2023

Choose a reason for hiding this comment

rparrett Jan 6, 2023

Choose a reason for hiding this comment

rparrett commented Jan 6, 2023

dvogel commented Jan 7, 2023

dvogel Jan 7, 2023

Choose a reason for hiding this comment

rparrett Jan 8, 2023

Choose a reason for hiding this comment

rparrett Jan 8, 2023 • edited Loading

Choose a reason for hiding this comment

rparrett Jan 8, 2023 • edited Loading

Choose a reason for hiding this comment

dvogel Jan 8, 2023

Choose a reason for hiding this comment

rparrett left a comment

Choose a reason for hiding this comment

bzm3r left a comment • edited Loading

Choose a reason for hiding this comment

dvogel commented Jan 11, 2023

rparrett left a comment • edited Loading

Choose a reason for hiding this comment

bzm3r commented Jan 11, 2023

bzm3r left a comment

Choose a reason for hiding this comment

dvogel commented Jan 13, 2023

rparrett commented Jan 13, 2023

bzm3r commented Jan 13, 2023

rparrett commented Jan 6, 2023 •

edited

Loading

rparrett Jan 8, 2023 •

edited

Loading

rparrett Jan 8, 2023 •

edited

Loading

bzm3r left a comment •

edited

Loading

rparrett left a comment •

edited

Loading