Ralith / sieve-tree

6 stars 1 forks source link

Use the correct level when inserting into the tree #12

Closed grovesNL closed 4 days ago

grovesNL commented 4 days ago

Fixes #11 Fixes #9

I'm not totally sure about this, so a close review would be greatly appreciated. This is my understanding after reviewing it for a while but I might have some details wrong.

The idea is that during insert, we look at the target's bounds and try to find a position for it. The bounds returned by bounds_from_world(self.granularity, &bounds) are fine. When we convert these tree bounds into cell coordinates (location::<GRID_EXPONENT>()) we do something like this:

let level = self.level::<GRID_EXPONENT>();
CellCoords::from_point(self.min, level)

level here accounts for GRID_EXPONENT so internally it ends up being level_for_extent(extent) + GRID_EXPONENT, which is 8 (level 6 + offset of 2) in this case.

In from_point we use the level we just calculated to figure out the extent:

let extent = cell_extent(level);
Self {
    min: point.map(|x| (x / extent) * extent),
    level,
}

but this is the extent for level 8 without accounting for GRID_EXPONENT. My understanding is that we actually want cell_extent(level - GRID_EXPONENT) here (i.e., the extent of level 6), so min would become the correct offset into the subdivided grid. At the same time though, we want to keep the level as 8 in the returned CellCoords.

So if we did want to account for GRID_EXPONENT when calculating min here, there are a few options:

  1. (this PR) We could inline location/from_point here and handle the levels/extent directly
    • We'd need to review the other places in the code base that use location/from_point and check if any of those places have the same problem.
  2. We could make location and from_point aware of GRID_EXPONENT and handle it internally, including the adjusted level (8) into the coordinate and using the extent from the unadjusted level (extent of level 6) for the min calculation.
    • These are used in several places in the code base but I don't have a good intuition yet about which call sites should be aware of GRID_EXPONENT
    • We could probably also make cell_extent GRID_EXPONENT aware which might simplify some places where we currently do cell_extent(level - GRID_EXPONENT)
    • If this is reasonable I think I'd prefer to do this instead of this PR
grovesNL commented 4 days ago

It looks like both update and remove use location, so I guess they probably have the same issue if this is correct.

Ralith commented 4 days ago

Good catch; there was indeed confusion between the coordinates of the target cell and the coordinates of the target node. The specifics aren't quite what you described here: insert and remove (but not update) both used the node coordinates to decide which cell within that node to insert into, which is obviously nonsense. Fixed in eedee1808b7cc6123246fa3df10762a0cf730dc9.