Fix incorrect Rect2i calculations: intersects and encloses

[Sauermann]

Correction of some border cases for Rect2i calculations.
Clarify expand documentation.
Added Unit tests for these cases.

resolves #57468

[lawnjelly]

Looks good to me.

[lawnjelly]

Just to note that this exclusive definition is the usual way of handling integer bounds in my experience. It is a matter of definition of course, but it's the same as an integer range in a for loop being:

for (int x=0; x<10; x++)

includes 0 to 9, but not 10.

One fun thing to mention with the plus one in the expand_to function, is that if you are doing this in any tight loops with thousands of points, it is possible to optimize by using the non plus one version, then adding one to the size at the end as a one off, rather than for every point.

[akien-mga]

That seems very weird to me. IMO the current behavior was correct, and if the documentation makes it sound like it should be different, then the documentation should be changed.

I would definitely be confused if Rect2i(0, 0, 10, 10).expand(Vector2i(20, 10)) returns Rect2i(0, 0, 21, 11). To me expand means "grow until the borders line up with the target point".

[Sauermann]

From your comment and from #57468 (comment) I find that the best way would be to keep the functionality of expand as it is and adjust the documentation accordingly.

[Sauermann]

One fun thing to mention with the plus one in the expand_to function, is that if you are doing this in any tight loops with thousands of points, it is possible to optimize by using the non plus one version, then adding one to the size at the end as a one off, rather than for every point.

In order to optimize this, iterate over all points while setting minx, miny, maxx, maxy and afterwards call expand_to twice. That way you don't calculate begin, end, position and size multiple times.

[akien-mga]

Thanks!

[lawnjelly]

Just to note that my review approval was based on the PR having the expand_to() being the "+1" definition, rather than Groud's version.

expand_to() is normally taken to mean "expand_to_include", that is the usual use of the function, to enlarge a bound to include a series of points. If you have a look at other libraries which use integer bounds you should see this (providing they use the exclusive definition of a bound).

Thus in this common usage, by definition, expand_to(foo) should now include the point "foo" and return has_point(foo) as true.

Problems with the non +1 version

The non +1 version of expand is sometimes used, particularly for optimization, but will not work well in the general case, there are a number of situations it can't deal with well.

Some examples

For instance, if you have an existing bound say in 1 dimension, (0 position, 10 size) and you want to expand that bound to include a point:

• If you want to expand the bound to include the point 15, in this existing version you would have to call:

expand_to(15);
size += 1;

• Whereas if you want to expand the bound to include the point 5, you would have to call:

expand_to(5);

You immediately have a problem, because the user needs to add code to deal with these two different situations.

• Thus you might start with a check at runtime something like the following:

old_bound = bound;
bound.expand_to(new_point);
if (bound.size > old_bound.size)
{
bound.size +=1;
}

However, there is a bigger problem.

• Consider if you wanted to add the point 10 to the existing bound:

old_bound = bound;
bound.expand_to(10);
// bound now includes numbers 0 to 9 inclusive
if (bound.size > old_bound.size)
{
// this if check fails, and the bound is now incorrect
bound.size += 1;
} 

• If you wanted this to work, your simple code would now become something like

old_bound = bound;
bound.expand_to(10);
if ((bound.size > old_bound.size) || ((bound.position + bound.size) == 10))
{
bound.size += 1;
}

This is one of a number of cases where this non +1 definition of expand_to breaks down.

+1 definition "just works"

With the standard definition of expand_to_include(foo), with +1, the code in all cases becomes:

bound.expand_to_include(10);

And it works in all situations.

Why the confusion?

I have a feeling there may be a confusion because using inclusive bounds (i.e. position 0, size 1, includes 0 and 1) which we do not use, then you don't need to apply the +1. But with exclusive, you do.

We should also consider the possibility that the existing implementation of the function from 3.x is simply a bug. We would have to ask @reduz (and he may not remember! 😁 ). It was only used from one place, atlas_packer, and it may well be causing a bug there as a result. As it hadn't been widely used, if it is a bug and non-intentional, it wouldn't have been recognised. I've made exactly the same bug myself in the past, but now recognise it as a special case that has to be dealt with.

Maybe a compromise could be to have two functions?

(1) expand_to_include() - This would be the +1 method, used to include the point in the bound
(2) expand_border() - Not quite sure how to name this, but this would be the non +1 method. I would discourage this use because it will cause errors of the sort above, and it probably should not be bound.

You could then simply go through the codebase with find_in_files, and see the intended usage, and call the appropriate function.

I've seen this exact situation come up before, and the solution used then was to fix the expand_to_include() to work correctly with +1, and then fix the bugs in any calling code that was built using the old version. But having a legacy function could work as a stop gap.

[Sauermann]

For reference, here are the locations in the code base, that use Rect2i.expand_to:

godot/editor/editor_atlas_packer.cpp

Line 103 in 2aee84c

aabb.expand_to(vertices[j]);

godot/editor/plugins/tiles/tile_map_editor.cpp

Line 831 in 2aee84c

drawn_grid_rect.expand_to(E.key);

Here point+(1,1) is explicitly included (and would benefit from the +1 version):

godot/editor/plugins/tiles/tile_map_editor.cpp

Lines 1559 to 1560 in 2aee84c

	encompassing_rect_coords.expand_to(E_cell->key() + Vector2i(1, 1));
	encompassing_rect_coords.expand_to(E_cell->key());

godot/editor/plugins/tiles/tile_map_editor.cpp

Line 2979 in 2aee84c

drawn_grid_rect.expand_to(E);

Herescreen_rect.grow(1) is used to enlarge the Rect2i after the expansions (and would benefit from the +1 version):

godot/editor/plugins/tiles/tile_map_editor.cpp

Lines 3855 to 3858 in 2aee84c

	screen_rect.expand_to(tile_map->world_to_map(xform_inv.xform(Vector2(0, screen_size.height))));
	screen_rect.expand_to(tile_map->world_to_map(xform_inv.xform(Vector2(screen_size.width, 0))));
	screen_rect.expand_to(tile_map->world_to_map(xform_inv.xform(screen_size)));
	screen_rect = screen_rect.grow(1);

godot/scene/2d/tile_map.cpp

Line 3377 in 2aee84c

used_rect_cache.expand_to(Vector2i(E.key.x, E.key.y));