Fix TileMap quadrant canvas item position not being local

[kleonc]

Should fix #86812, see #86812 (comment) for details.

[groud]

Thanks for the contribution.

I am not sure this makes sense, and not sure the original issue either. I think this will break anyway as soon as you have tiles that are bigger than a cell, with either big tiles, or a texture region size configured in the atlas bigger than the tile size.

The "top left corner" of the CanvasItem cannot be determined unless you know which tile is in the top left cell, which is a problem as all vertex coordinates would change depending on it.

For me, the original behavior is expected and preferable.

[kleonc]

First of all I had a bug in this PR as 0.5 * tile_set->get_tile_size() does int * Vector2i aka result is always zero... 🙃 Pushed corrected version with a cast.

I am not sure this makes sense, and not sure the original issue either. I think this will break anyway as soon as you have tiles that are bigger than a cell, with either big tiles, or a texture region size configured in the atlas bigger than the tile size.

The "top left corner" of the CanvasItem cannot be determined unless you know which tile is in the top left cell, which is a problem as all vertex coordinates would change depending on it.

Oh, I meant the top left of the bounding rect of the quadrant's "first cell" (cell at quad_size * quadrant_coords map coords). AFAICT this depends only on the TileSet settings + TileMap quadrant size, such quadrant origin doesn't depend on whether/what tiles are rendered (texture size doesn't affect cell counds).

Like for isometric or hexagonal shapes using VERTEX in the shader seems kinda pointless anyway since all tiles are rendered as quads regardless of the shape and thus for hex/iso such quads overlap the neighbors / their vertices are outside of the actual cell area.

The most important part is that when opening this PR I haven't remembered about render_mode world_vertex_coords (clayjohn reminded about it right when I was opening this PR), I've only remembered about render_mode skip_vertex_transform at that time. 🙃
With render_mode world_vertex_coords available probably most use cases are already covered. In case someone wants local coords in the shader instead of the global ones then it's possible to pass tilemap.global_transform.affine_inverse() as a shader uniform and use it for converting global to local. So also not a problem. And I dont' see why ever in-quadrant coords would be needed over local coords (but if needed they could also be manually calculated within the shader from the local coords).

I still think the current positions set for quadrant canvas items are strange / confusing though. But with this PR I see users still being confused, just differently. 😄

	v4.3.dev1.official [9d1cbab]	this PR
fract(VERTEX / 128.0)

So in case you still disagree with this PR then please close / give a closing note in #86812 as in such case there's nothing to be done about it.

And of course thanks for the feedback!

[groud]

Oh, I meant the top left of the bounding rect of the quadrant's "first cell" (cell at quad_size * quadrant_coords map coords). AFAICT this depends only on the TileSet settings + TileMap quadrant size, such quadrant origin doesn't depend on whether/what tiles are rendered (texture size doesn't affect cell counds).

What I mean is that this value does not mean a lot. If you have a texture region size bigger than the grid, then all quadrants will end up unaligned with you textures anyway, and the origin won't be a the "correct" position.

To make it clear, my point is that there's not a lot more sense to align the origin of canvas items to the top-left corner of the cell than there is to align it to the center of the top-left cell. The center of the cell is in general consistent with the way the TileMap node deals with cells. So yeah, it might be a bit weird at first, but all offsets/positions in the tile configurations are done according to the center of the cell. So I think we should keep things consistent there.

tilemap.global_transform.affine_inverse() as a shader uniform and use it for converting global to local

I wonder if we could provide some built-in shader variables for the TileMap node. That could be a better approach IMO.

[Zireael07]

I wonder if we could provide some built-in shader variables for the TileMap node.

This would be a good idea in general

[kleonc]

What I mean is that this value does not mean a lot. If you have a texture region size bigger than the grid, then all quadrants will end up unaligned with you textures anyway, and the origin won't be a the "correct" position.

To make it clear, my point is that there's not a lot more sense to align the origin of canvas items to the top-left corner of the cell than there is to align it to the center of the top-left cell. The center of the cell is in general consistent with the way the TileMap node deals with cells. So yeah, it might be a bit weird at first, but all offsets/positions in the tile configurations are done according to the center of the cell. So I think we should keep things consistent there.

Yeah, I agree. Pretty much the only advantage I can see of the top-left over the center is that it seems nicer for the most basic case of a rectangular grid. For any other setup there will always be something off. For such other setups the center indeed seems like a more reliable reference point. And consistency is a good point in favor of the center.

Just to be clear: currently (before this PR) it's neither the top-left corner nor the center of the cell, it's the map-coords of the cell (see #86812 (comment)).
Initially you've said "the original behavior is expected and preferable", now you've said the center is better. I'm not sure if you meant only "center is better than top-left" or "center is better than both top-left and current behavior".

Anyway, I agree in general center makes more sense than top-left, so I've pushed a change so it's center now in this PR (if needed it's easy to offset manually by the half cell size anyway).

tilemap.global_transform.affine_inverse() as a shader uniform and use it for converting global to local

I wonder if we could provide some built-in shader variables for the TileMap node. That could be a better approach IMO.

AFAICT from the rendering POV a TileMap is just an arbitrary canvas item in the middle of some canvas item parent-children hierarchy. And if I get it right you're suggesting to somehow provide some partial transforms to such specific canvas item (TileMap) to be available in a shader run for a different (descendant) canvas item (quadrant)? I have no idea if this is feasible / how would it meant to be done.

Of course I agree having built-in transforms like (quadrant to TileMap / TileMap to global / their inverses) would be nice from the user POV. But yeah, I don't see the "how". 🙃

[groud]

Just to be clear: currently (before this PR) it's neither the top-left corner nor the center of the cell, it's the map-coords of the cell (see #86812 (comment)).

Oh damn, you are right. Indeed that was the original intent for that code. I just focused on the "half a cell" offset but not on that. using map_to_local is indeed a correct fix.

AFAICT from the rendering POV a TileMap is just an arbitrary canvas item in the middle of some canvas item parent-children hierarchy. And if I get it right you're suggesting to somehow provide some partial transforms to such specific canvas item (TileMap) to be available in a shader run for a different (descendant) canvas item (quadrant)? I have no idea if this is feasible / how would it meant to be done.

Yeah I'm not sure either. It's unclear to me what could be done, I was suggesting this as a general idea if it could help.
But yeah, I guess it would require a proposal.

Anyway, in its current state the PR looks good. Approved.

[akien-mga]

Thanks!