Fix#23250 rounding error in world_to_map function in tile_map

[soockee]

[deprecated]Problem: Returning Vector2D always get round down. Vector2D round off is the solution.

Used this script for testing:

extends Node2D
func _ready():	
	var tiles = $TileMap.get_used_cells()
	var count = tiles.size()
	print(count)
	if !tiles.empty():
	   for i in count:
		   var before = tiles[i]
		   var world = $TileMap.map_to_world(tiles[i])	  
		   var after = $TileMap.world_to_map(world)
		   if after != before:
			    print("Test Failed:",before,"!=", after,"World=",world)
		   else:
			    print("Test Succeded:",before,"==",after,"World=",world)
	else:
		print("empty")

Edit:
Problem is indeed a rounding error. The Function affine_inverse in transform_2d.cpp got problems with TileMap-cell_sizes which are one of those numbers:

the variable idet doesn't evaluate to the desired number
as example:

det= 400
idet = 1.0 / det #idet is about 0.0024999... but should be 0.0025

The commit provides some workaround in a way that only some of these digits are considered significant to avoid the representational error.
Bug-Reproduction Project
from #23250 (comment)
bug.zip

[Zylann]

If I make a tilemap with cell size of 32x32, and run world_to_map at (30, 30), will this return the expected result (0, 0) ? Please test the other use cases, these two functions aren't true inverses.

[soockee]

Yes u are right. I didn't test other use-cases and 32x32 map with points between (16,16) and (31,31) are not correct since they get rounded up to (1,1) which seems to be invalid as you mentioned. Thanks for pointing that out.

[reduz]

Sorry this fix does not make any sense, transform2D is used in plenty of other things and this would break it. Why not just rounding the result in tile map?

[soockee]

ok this actuall fix might not be very well thought out. I still wonder why
idet = 1.0 / det
with det = 400
is not resulting to 0.0025 but 0.0024999... which then gets round down, because of floor() in world_to_map.
I'm simply not expacting that (both values of type real_t by the way) 1.0 / 400 does not result to 0.0025

[Zireael07]

@soockee: the answer is float precision...

[akien-mga]

That's completely normal with floating-point arithmetic.

[soockee]

Yes i understand. Just asking how would you handle it in the world_to_map function. Guess my solution isn't going to work. My thought was: There is this float precision which causes the number to be just a little bit off. It returns to the world_to_map function where it's used again. But because it's just that little bit of the result of world_to_map is not what we expect -> issue
Since it's a "beginners" label, i am simply asking for advice of experienced developers :)

[Zireael07]

Off my cuff, I would check whether the number falls in certain range (e.g. 0.25 +- epsilon or 0.75 +- epsilon) and decide whether to floor() or ceil() based on the range it's in.
Note: not sure if Godot has built-in epsilon just like PI.

[akien-mga]

We need to floor as everything in the range [n, n-1[ has to yield n (where n is x / TILE_SIZE). I'm working on a fix, adding a small offset before flooring should do the trick.

[Zylann]

Another way to solve this would be to transform the world coordinate using a Transform which doesn't include cell size. This gives a tilemap-space coordinate in pixels. You round that to an integer, and only then, you unscale the cell size to return a grid-space coordinate. Only drawback is, you may not make a tilemap with non-integer cell sizes ;p

[akien-mga]

@Zylann Well isn't this what's done already? The issue is "you unscale the cell size to return a grid-space coordinate." -> that's where the precision error can mean that you're off by one when going back to an int cell coordinate.

[Zylann]

@akien-mga no that wasn't the case. The original code is using only one transform which gave the result in grid coordinate directly (see the switch for half_offset, it applies 0.5, that's already not in pixels). My idea follows two steps instead, where the first step can be rounded safely because it's in pixel space (provided we don't have non-integer cell size), and the second step would run through integer math, so no possible floating point error. The issue with the first fix proposal is that it was rounding too late, on a grid-space coordinate.
(I'm not sure how see again the original fix code tho?)

[akien-mga]

@Zylann Well if you could implement your idea, that would be nice. Even with a hack to workaround the precision error before calling floor, the current logic is limited in what tile size it can support at all (e.g. x = 19999 and TILE_SIZE = 20000 already give a cell coordinate of 0.99995, that's close to CMP_EPSILON = 0.00001).

If we don't care about huge cell sizes, here's a brute force fix:

diff --git a/scene/2d/tile_map.cpp b/scene/2d/tile_map.cpp
index 67e25ec50..a7ac2bb98 100644
--- a/scene/2d/tile_map.cpp
+++ b/scene/2d/tile_map.cpp
@@ -1445,6 +1445,11 @@ Vector2 TileMap::world_to_map(const Vector2 &p_pos) const {
 		default: {}
 	}
 
+	// Account for precision errors on the border (GH-23250).
+	// 0.00005 is 5*CMP_EPSILON, results would start being unpredictible if
+	// cell size is > 15,000, but we can hardly have more precision anyway with
+	// floating point.
+	ret += Vector2(0.00005, 0.00005);
 	return ret.floor();
 }

[akien-mga]

We should probably continue the discussion in the original issue #23250 as this PR changing Transform2D won't be merged.