Fix width and height usage

[oliver-foggin]

Updating the width/height issues.

Currently width and height are used to mean one of two things...

1. The distance from the centre of a node to the edge.
2. The distance from one edge of a node to the opposite edge.

The second use is more consistent with how "width" and "height" are used as general concepts so I am opting to use this as the new single definition.

Another small fix I will apply is the comparison check to determine if a point is "in" a node. Currently, a point can belong to multiple sibling nodes (i.e. nodes on the same level). I will update so that each point can only belong to a single node.

[crhallberg]

This looks good so far! I’ll give it a closer review soon.

[oliver-foggin]

@crhallberg This addresses all the width and height issues now. There were a few in the way that points were evaluated and then also in how the subdivide logic worked.

Once those were fixed all of the corresponding tests that used the logic of width equals distance from centre to edge were fixed with the updated logic of width equals distance from left edge to right edge.

With these changes it should make it possible to implement the k-nearest-neighbours which I'll have a go at next.

👍🏻

[oliver-foggin]

Ah, just seen there's still an update for the intersects. Working on that now.

[oliver-foggin]

OK, done now. Should have everything covered I think. 👍🏻

[oliver-foggin]

I'm going to make another update to change the way the contains is used for querying.

[oliver-foggin]

@crhallberg I've now added a new queryContains function which will include any points along any edge. This is used only for querying the quad tree rather than for adding points. I'm not 100% happy with the names of contains and queryContains so happy to take suggestions.

Also added two new tests that highlight the issue.

[oliverfoggin]

@crhallberg I've now added a new queryContains function which will include any points along any edge. This is used only for querying the quad tree rather than for adding points. I'm not 100% happy with the names of contains and queryContains so happy to take suggestions.

Also added two new tests that highlight the issue.

I was thinking maybe queryContains and pointBelongsIn or something like that?

[crhallberg]

I'm not sure I understand the difference between the comparisons. I thought the point was that if we consistently use >= and < then nothing will get double flagged.

EDIT: If we have differing cases for placing and querying, I feel like we're going to get inconsistent results. A point on the edge will not be placed inside but will be searched for inside that region?

My opinion is that we shouldn't change the name of contains as it would be breaking and also make Rectangle and Circle diverge.

[oliverfoggin]

I'm not sure I understand the difference between the comparisons. I thought the point was that if we consistently use >= and < then nothing will get double flagged.

EDIT: If we have differing cases for placing and querying, I feel like we're going to get inconsistent results. A point on the edge will not be placed inside but will be searched for inside that region?

My opinion is that we shouldn't change the name of contains as it would be breaking and also make Rectangle and Circle diverge.

Well the difference is in why it is used.

In the insert code you have this at the end...

return northeast.insert || northwest.insert || southeast.insert || southwest.insert

The check for adding any point should only allow one of these inserts to work. With the old logic, if a point was on the border of northeast and northwest (or worse, the point where all four meet) then it would get added to multiple child regions.

So, we need to make sure that when adding a point it can only ever be added to a single child region.

However, when querying, we no longer have this restriction. And in fact shouldn't want it.

When running a query we're not passing in a child region to ask if it should contain a point or not. We're asking if an arbitrary region encompasses that point.

For example.

If we had a quad tree like (50, 50, 100, 100) I.e. one that goes from (0,0) to (100,100).

When adding the point (25, 50) during a subdivision. We want it to always go into the northeast region (and never the northwest region).

However, if we run a query of the region (25, 25, 50, 50) I.e. from (0,0) to (50,50) (which is actually the same region as the northwest of the quad tree) then it should definitely find that point. Even though it would not be added to a region of the same shape due to the region uniqueness.

Whilst it may at first appear that the two are the same check, they actually are subtly different.

Luckily, the two cases are completely distinct and so using different names would be ok. For example, you would never be adding a node to a region that is a circle. You would only ever be querying the quad tree with a circle.

Hope that makes sense.

[crhallberg]

That does make sense! Thank you for the explanation!

How would you feel about the names contains for query and shouldContain for inserting/subdividing?

[oliverfoggin]

That does make sense! Thank you for the explanation!

How would you feel about the names contains for query and shouldContain for inserting/subdividing?

Those are perfect yes. I'll make that change.

Thanks 👍🏻

[crhallberg]

Your explanation also reveals why the check is inclusive on the right and bottom: it escapes on the OR.

return false || 0 || 7 || 8; // 7
return alwaysFalse() || alwaysTrue() || veryExpensiveTrue(); // veryExpensiveTrue was never run

OR statements stop evaluating once they get a true.

return northeast.insert || northwest.insert || southeast.insert || southwest.insert

A point on the border between NE and NW will be grabbed by NE because it checks first and northwest.insert is never run.

Knowing this, I think I'd rather keep the one inclusive contains call for both inserting and querying but the rest of the width/height work is definitely crucial and it's amazing it hasn't caused issues yet.

[oliver-foggin]

Your explanation also reveals why the check is inclusive on the right and bottom: it escapes on the OR.

return false || 0 || 7 || 8; // 7
return alwaysFalse() || alwaysTrue() || veryExpensiveTrue(); // veryExpensiveTrue was never run

OR statements stop evaluating once they get a true.

return northeast.insert || northwest.insert || southeast.insert || southwest.insert

A point on the border between NE and NW will be grabbed by NE because it checks first and northwest.insert is never run.

Knowing this, I think I'd rather keep the one inclusive contains call for both inserting and querying but the rest of the width/height work is definitely crucial and it's amazing it hasn't caused issues yet.

Ah yeah, good shout. I'd completely skipped over that.

However, if I go ahead with the "remove named children" I think the update may be required. 😄

I'll go back to the single contains function and put the logic to be inclusive at all edges. I can address any other updates in other PRs if necessary 👍🏻

[oliver-foggin]

it's amazing it hasn't caused issues yet

Yes, I'm kind of amazed. When you work through the logic of the subdivision and look at the region covered by it you find out it's very different from what it should be.

With the original logic a region (black) and it's sub divisions (red) would look something like this...

[oliverfoggin]

I think a combination of the inconsistencies came together to account for the errors in each other and cancel out (to a certain extent).

But if we want to update to use k-nearest-neighbours then we need to make sure that everything is working as it should.

[oliver-foggin]

In fact, yeah, looking back at the old subdivision and contains logic. In the above photo the contains would return true for the purple regions I've added here...

So, contains would return true for the green point in the NW region. Even though it wasn't actually contained in the NW region. And any testing on this would look fine.

However, contains would also return true for the blue point in the NW region. And I can only imagine that there wasn't a test written for this as it would be an odd thing to test... but also a difficult scenario to set up.

[crhallberg]

I've finally found the time to go step-by-step through the old code and I think your drawings are correct:

• contains and intersects are both using width/height like a radius: checking an area twice as large as the edge getters (left(), right(), etc.) have you believe.

  • This PR fixes this with the use of .left (etc), since we want width/height to be treated as the total width/height, not from the center.
  • You can see this in static create() where the default is to create a QuadTree the same size as the sketch.
  • quadtree.create.spec.js didn't catch this because it checks the edge getters, which aren't used in the code.
  • quadtree.spec.js (line 134) should be updated to put a point just outside the expected bounds
    • At (115, 200) and/or (100, 230), since the range is (90-110),(175-255)

• subdivide is broken as you say: the quads are too far from the center.

  • This method is broken but has been saved by the above double-sized checking.
  • Since all four quads are always made at the same time, I don't agree with the refactor to another location, the original format was clearer, IMO.

All in all, great work so far! I think you've caught all the instances that I can see. I have some nitpicks about the tests but they are outside the scope here.

[oliverfoggin]

Cool, thanks for the update and summary.

I refactored the subdivide as it is done in two places. Once in the actual subdivide and once in the JSON decoding function.

Happy to put that back though if you would prefer. And yes, I agree with the test. I'll add another one in 👍🏻

[crhallberg]

You're right. Excellent! What's left that I can dig into?

[oliver-foggin]

@crhallberg I updated that test and added an additional test to test points on the boundary.

[crhallberg]

Seems ready to go! Are you ready?

[oliver-foggin]

@crhallberg yeah, I don't think there are any more updates for this PR from me 👍🏻

[oliver-foggin]

@crhallberg I'm not able to merge it. I think you will have to merge the PR yourself. 👍🏻

[crhallberg]

Thanks so much!

[oliverfoggin]

Thanks so much!

Awesome thanks!

I'll get on to looking at the k-nearest-neighbours now. See if we can get something interesting working with that.

👍🏻