mourner / rbush

RBush — a high-performance JavaScript R-tree-based 2D spatial index for points and rectangles
MIT License
2.48k stars 237 forks source link

Invalid collision on maxX/Y (pixel-perfect collision false positive) #108

Closed Llorx closed 4 years ago

Llorx commented 4 years ago

Lets say we have one box with this position:

let box1 = {
  minX: 10,
  minY: 10,
  maxX: 15,
  maxY: 15
};

If I query rbush with a box that do not collide with this position, it actually returns box1:

let box2 = {
  minX: 15,
  minY: 15,
  maxX: 30,
  maxY: 30
};
let bush = new rbush();
bush.load([box1]);
console.log(bush.search(box2));

fiddle

This is the intersection: image

As you can see if I zoom-in, they do not overlap: image

I guess that the maxX/Y comparison against the minX/Y should be "less than" instead of "less than or equal" here

I calculate the maxX being X + width (like everyone do, including the demos). Right now rbush is more a "occupied pixel including maxX/Y" instead of the "starting pixel inclusive and ending pixel exclusive" which is the norm for box collisions actually (to avoid false positives like the above).

mourner commented 4 years ago

instead of the "starting pixel inclusive and ending pixel exclusive" which is the norm for box collisions actually

I don't think that applies to spatial indices. Spatial data structures don't involve rendering concepts, they are strictly about geometries. A bounding box of an object is defined by the minimum and maximum values on axes, which imply inclusive relationship. Also, if geometries "touch", they are considered intersecting too, which is an established convention across many projects, e.g. PostGIS ST_Intersects.

Llorx commented 4 years ago

Hm, but this is not default behaviour, for example, in browsers. I get your point with spatial data structures by the way.

Can we, maybe, do something with this? Maybe just create a RBush instance with an option to avoid "touching intersections" which will use one method or another depending on this option. Something like { touching: false }. This will override the "insersects" function with "intersectsNoTouch" (or whatever) so any call to "intersects" will behave properly for any rendering system, which rbush is used a lot with.

@mourner I can do a mockup of what I have in mind for you to check.

IvanSanchez commented 4 years ago

Within the GIS field (and rbush was made for GIS applications AFAIK), the meaning of "intersect" is well-defined as per DE-9IM: having at least one point in common.

I calculate the maxX being X + width (like everyone do, including the demos)

No you don't! If you're thinking in pixels (or in 2D graphics APIs like fillRect), then maxX = minX + width - 1. This is proven by thinking about the trivial case of a 1x1 rectangle, where the pixels for the four corners are the same (and are not offset by one).

So if you have anything resembling a dot matrix, you should think (again?) about the meaning of your coordinates: do they mean the address of a pixel (so the center of every pixel has integer coordinates, and a zero-by-zero rectangle occupies one pixel), or do they mean the corners of pixels (so the pixel spans a one-by-one area, and its center is at coordinates something-point-five)?