Add physical_tile_size field to determine how large each tile is rendered in-world

[SirHall]

As bevy_ecs_tilemap stands now, each tile's size in-world is tied to the pixel-size of the tile in the source atlas.
If I however want my tiles to be of unit size or any particular scale that I require, this is not currently possible.
With this pull request, a user can now set physical_tile_size to determine how large a tile should be rendered in-world.

This should also close #337.

[bzm3r]

I am not sure about the name physical_tile_size. physical is a bit of an overloaded term.

I think TilemapTileSize should behave exactly like the new TilemapPhysicalTileSize.

Instead, there should be a separate type called SourceTileSize, which specifies how large a tile is in the source texture. It should not be a field on TilemapBundle, instead the variants of TilemapTexture should be structs which have a field tile_size: SourceTileSize

TilemapTileSize should be an option on TilemapBundle. Currently we have:

bevy_ecs_tilemap/src/lib.rs

Line 103 in cac7598

pub tile_size: TilemapTileSize,

Instead this should be pub tile_size: Option<TilemapTileSize>

If the user wants to specify an explicit TilemapTileSize, they can do so by providing Some(TilemapTileSize { ... }), otherwise the TilemapTileSize component should be inserted based on the SourceTileSize provided for the TilemapTexture

The shaders should get two pieces of info: the SourceTileSize, and the TilemapTileSize

Let me know if this makes sense to you? I think implementing this shouldn't be too hard based on what you've written so far; it will require some variable renaming and some work on TilemapTexture, but will be a clearer solution overall?

@StarArawn @rparrett Let me know if the above review suggestion makes sense to you guys too.

[SirHall]

Yeah that's fine, to me 'physical' meant how large it would 'physically' appear in the game-world.
I'm now thinking it may actually be even clearer to have one be TilemapSourceTileSize and the in-world one be TilemapPhysicalTileSize to clearly separate both - though I'm fine with any reasonable naming scheme.
I don't have time immediately to go over it so I'll probably have to do it the day after tomorrow for me.

[bzm3r]

I would prefer TilemapSourceTileSize and TilemapTileSize. Or TextureTileSize and TilemapTileSize.

I am just not a fan of "physical" because it comes up a lot in gpu stuff with fairly different meanings. In programming more generally, the term "physical" is often contrasted with "logical"; if there is a TilemapPhysicalTileSize, is there a TilemapLogicalTileSize?

[rparrett]

Agree that TilemapPhysicalTileSize strikes me as referring to physical pixels on a screen.

Instead this should be pub tile_size: Option<TilemapTileSize>

I don't think bevy will let us do that.

[SirHall]

I do think the comparison to GPU architecture is being a little pedantic - but I'm fine with any reasonable option, do let me know with the two names once it's settled.

[juzi5201314]

Useful feature, can we still hope to get it? Or can such a function be achieved on existing code?

[SirHall]

Useful feature, can we still hope to get it? Or can such a function be achieved on existing code?

I have been quite busy for a while recently and only a few days thought of updating this PR to finally get it merged in, ideally I can get this merged in shortly.

[SirHall]

I think I'll wait for the upgrade to bevy 0.12 to get merged in so as to reduce the amount of rework required.

[MeoMix]

just FYI I am working on addressing the PRs concerns in my own branch, should have it done tmmw, would like to get it in for the next release

@bzm3r if I put SourceTileSize into TilemapTexture then it needs to derive Hash because TilemapTexture derives Hash

TilemapTileSize x/y is f32 but f32 isn't valid for deriving Hash - needs to be u32

Is that acceptable? I assume no? But unclear how to adjust architecture to accommodate.

Also

Instead this should be pub tile_size: Option

As this isn't possible - what is the desired solution? Is there even an issue here beyond just expecting users to be extra explicit in their sizing?

Also

The shaders should get two pieces of info: the SourceTileSize, and the TilemapTileSize

Can you clarify this? The functionality seems to work well as-is. It's not clear to me if you're saying we're interested in providing additional values to the shaders. If you feel we are - why is that necessary? Or is the fact that the shaders (currently in this PR) access tilemap_data.grid_size and tilemap_data.physical_tile_size sufficient?

[SirHall]

Sorry for the long hiatus, this has been reworked, physical renamed to in_world (as I feel this causes the least confusion) and should be good to merge.

[SirHall]

I don't know if @bzm3r is still active, could we have a new reviewer for this PR?