Implements first draft of quadtree lookup implemented via BoundingBox #12

DerLando · 2020-01-30T15:45:32Z

In the first draft for this I implemented the quadtree insertion via BoundingBox. I would have liked to implement the Spatial trait from aabb for Bounded but i could not get it to work without using Box<dyn Bounded>

It would probably make more sense to implement Spatial for DrawingObject as this is for now the main thing we want to query for.

Michael-F-Bryan · 2020-01-31T07:16:57Z

It would probably make more sense to implement Spatial for DrawingObject as this is for now the main thing we want to query for.

We can't add a DrawingObject to the quad tree directly because it's owned by the ECS storage, so you'll need to store an Entity.

I imagine the object we'd be adding to the quad tree would be a pair of a BoundingBox and an Entity.

struct Thing {
  bounds: BoundingBox,
  entity: Entity,
}

I would have liked to implement the Spatial trait from aabb for Bounded but i could not get it to work without using Box

~~Can you write something like this?v We define Bounded so the compiler shouldn't be complaining about the orphan rules. Can you show me the error message?~~

EDIT: I had another look. We're trying to say any object implementing our Bounded trait should also implement Spatial, but that'll fail because of the orphan rules. It'd be trivial for a 3rd party that depends on both aabb to have a manual implementation of Spatial, then when they pull in arcs and implement Bounded the compiler will get all confused ("coherence").

I don't think we'd be able to add that generic impl for Spatial, although if we aren't adding DrawingObjects to the quad tree we may not need it.

With this it is now possible to insert *Entities* from *World* into a quadtree And query for them using *BoundingBox* queries

DerLando · 2020-01-31T10:22:18Z

Using an intermediate SpatialEntity it is possible to insert entities in the quadtree 🙂

and keep track of changes to entities within it

DerLando · 2020-01-31T15:34:21Z

Oof this commit got quite wordy

Would it make sense to implement an API similar to NameTable that lets us call

let space = world.read_resource::<SpaceTree>();
space.query_point();
space.query_region();

instead of the hacky methods on the system itself?

DerLando · 2020-01-31T15:36:17Z

arcs/src/systems/spatial_relation.rs

+        }
+    }
+
+    pub fn query_point(&self, pt: Vector) -> Option<Vec<Entity>> {


This should probably be possible to be called from somewhere else

with a similar structure to the *NameTableBookkeeping*

…Relation* system now

DerLando · 2020-02-01T11:58:05Z

I think I found a way around the problem of my branch being dependency-locked to

euclid = "0.19.8"

We should be able to reference different versions using this:
rust-lang/cargo#4953 (comment)

euclid = "0.20.7"
quadtree_euclid = { version = "0.19.8", registry = "custom", package = "euclid" }

Michael-F-Bryan · 2020-02-01T12:04:08Z

I'm about to land #13 which uses euclid (v0.20.7) types for everything. Is that going to break much for this PR?

DerLando · 2020-02-01T12:07:12Z

It's going to break all the tests and some methods on Space, but it shouldn't be to hard to clean up those afterwards for me

…e future

Michael-F-Bryan

Thanks for getting the ball rolling on this.

After looking at the code I'm not overly satisfied with aabb-quadtree's API. It feels a bit restrictive and doesn't necessarily match the way I've needed to use these sorts of data structures in the past.

What did it feel like trying to wrap the QuadTree when writing Space?

Michael-F-Bryan · 2020-02-01T12:49:13Z

arcs/src/components/spatial_entity.rs

+    fn default_tree() -> SpatialTree{
+        // Initialize quadtree
+        let size = BoundingBox::new(
+            Vector::new(-Self::WORLD_RADIUS, -Self::WORLD_RADIUS),


I'm not a massive fan of telling the SpatialTree the size of the world up-front... Will the code panic if we choose the wrong number and try to place something outside of the space? And what happens if the space is a lot bigger than it needs to be, will we consume an unnecessary amount of memory?

As i commented here, the trees bounding_box is hard-coded on initialization. If we try to insert something outside of those bounds afterwards it will panic.
I'm not a fan of how the aabb_quadtree handles this and I only see 3 options:

Check all insertions in our API and rebuild the tree with a big enough bounding box every time (seems really inefficient)

Allocate a very big world size (also inefficient)

Use another Quadtree implementation

I did some more thinking on this:

Space needs to implement Default so we can use it as a Resource, this inevitely means we have to define some default starting parameters, as we can not supply arguments to Space::default().

Memory-wise we should still be quite efficient, as very large empty space in a quad-tree should take the same amount of memory as small, filled spaces

To be extra-safe I could write a wrapper around aabb_quadtree.insert() which detects objects outside the bounds of the tree-root and resize the tree if needed

Resizing basically means re-building the whole tree again, but with a large enough BoundingBox so the newly added SpatialEntity fits in

Space needs to implement Default so we can use it as a Resource, this inevitely means we have to define some default starting parameters, as we can not supply arguments to Space::default().

Could we add Space as a resource in the bookkeeping system's setup() method then use ReadExpect when trying to access it?

Resizing basically means re-building the whole tree again, but with a large enough BoundingBox so the newly added SpatialEntity fits in

That's annoying.

I was kinda hoping you could implement a resize operation by taking the quad tree's "head" node and wrapping it in a larger node, almost like the tree equivalent of adding an item to the front of a linked list. You'd be left with a really unbalanced tree (the entire previous tree is in one quadrant of the tree's head node), but it turns a resize into an O(1) operation where we just copy some pointers around.

Could we add Space as a resource in the bookkeeping system's setup() method then use ReadExpect when trying to access it?

This seems like an unnecessary mix of two unrelated systems. So I'd rather stick to Space being it's own system

almost like the tree equivalent of adding an item to the front of a linked list.
There is nothing in the API that woudl allow for this.

It really looks like we need a better backend for our Space

Michael-F-Bryan · 2020-02-01T12:50:20Z

arcs/src/components/spatial_entity.rs

+            ).aabb();
+        let quadtree: SpatialTree = QuadTree::new(
+            size,
+            true,


We should probably pull these out into constants, because true, 4, 16, 8, 4 doesn't really mean anything.

arcs/src/components/spatial_entity.rs

Michael-F-Bryan · 2020-02-01T12:54:06Z

arcs/src/components/spatial_entity.rs

+    }
+
+    pub fn insert(&mut self, spatial: SpatialEntity) {
+        let entity_id = spatial.entity.id();


Why are we using id() here instead of the full Entity? An Entity also contains a Generation so you can't get the equivalent of a use-after-free when an entity gets deleted and specs allocates the same Index to an entity created later on.

Actually that makes more sense!

arcs/src/components/spatial_entity.rs

arcs/src/systems/spatial_relation.rs

Michael-F-Bryan · 2020-02-01T13:12:59Z

arcs/src/systems/spatial_relation.rs

+        let mut system = SpatialRelation::new(&world);
+        System::setup(&mut system, &mut world);
+
+        // make some changes after the initial setup


As well as object creation, we may want to make sure object modification and deletion are detected too.

I'm a bit stuck on that part @Michael-F-Bryan ,

Similar as to the test for name_table_bookkeeping after we read a resource from world we can no longer borrow world as mutable.

let space = world.read_resource::<Space>(); // <- world is immutably borrowed here world.delete_entity(first).unwrap(); // <- We would mutably borrow here

I can't seem to find a nice pattern/api for either dropping space or another way of letting us do queries but also still being able to manipulate the world afterwards.

How would you implement/test this for NameTable the implementation for Space should be similar

Hmm... I guess you could fetch the space every time you need it instead of putting it in a local variable.

We should only need the space after the second system.run_now() when we do our assertions though, shouldn't we?

Yes, never storing Space in a variable solves this limitation.

Michael-F-Bryan · 2020-02-01T13:13:37Z

arcs/src/systems/spatial_relation.rs

+        let space = world.read_resource::<Space>();
+
+        let query = space.query_point(Vector::new(3.0, -0.5));
+        assert!(query != None);


This could be written as assert_ne!(query, None) or assert!(query.is_some()).

Michael-F-Bryan · 2020-02-01T13:17:18Z

arcs/src/systems/spatial_relation.rs

+
+        let query = space.query_point(Vector::new(3.0, -0.5));
+        assert!(query != None);
+        assert_eq!(query.unwrap().len(), 1);


Hmm... After seeing how a typical caller will be using the query, does it actually make sense to return an Option<Vec<_>>?

If we return an empty Vec (note: it's guaranteed that empty vecs don't allocate) when the query has no results, the query_*() methods should be a lot more ergonomic.

Also, semantically returning an empty Vec means your query completed but nothing was found. Which kinda makes sense?

True that makes more sense!

…rnung *impl Iterator*

…ants

added panics for unexpected outcomes

…ox* events

… implement-quadtree-lookup

…om space when encountering *ComponentEvent::Removed*

out-of-bounds at a later stage

… implement-quadtree-lookup

DerLando · 2020-02-03T22:00:54Z

I Should have taken care of all comments, some further thought:

The aabb_quadtree seems not mature enough for our use case, also there does not seem to be any crate we could use for this, so we will probaly need to implement a better tree at some point
I tried to structure Space in a way that makes it easy enough to replace the quadtree backend at a later stage
I think i found a bug inside of SyncBounds: bounds.remove() is never called inside of SyncBounds.run(), this led to some confusion in my testing of the removal of entities from Space. I will create a separate issue for this

Michael-F-Bryan · 2020-02-04T12:13:10Z

The aabb_quadtree seems not mature enough for our use case, also there does not seem to be any crate we could use for this, so we will probaly need to implement a better tree at some point

I agree with you there, but I'd like to see how far we can get without implementing our own. The longer we wait, the more we'll know what sort of API we want.

Michael-F-Bryan · 2020-02-04T12:57:48Z

I found an interesting article called A dive into spatial search algorithms.

They propose a nice way of retrieving query results in order...

An intuitive observation: when we search a particular set of boxes for K closest points, the boxes that are closer to the query point are more likely to have the points we look for. To use that to our advantage, we start our search at the top level by arranging the biggest boxes into a queue in the order from nearest to farthest:

Next, we “open” the nearest box, removing it from the queue and putting all its children (smaller boxes) back into the queue alongside the bigger ones:

We go on like that, opening the nearest box each time and putting its children back into the queue. When the nearest item removed from the queue is an actual point, it’s guaranteed to be the nearest point. The second point from the top of the queue will be second nearest, and so on.

This comes from the fact that all boxes we didn’t yet open only contain points that are farther than the distance to this box, so any points we pull from the queue will be nearer than points in any remaining boxes:

If our spatial tree is well balanced (meaning the branches are approximately the same size), we’ll only have to deal with a handful of boxes — leaving all the rest unopened during the search. This makes this algorithm extremely fast.

DerLando · 2020-02-04T13:08:53Z

That is indeed an interesing query algorithm!

I wrote a toy quadtree alongside implementing aabb_quadtree to understand the data structure better, I should implement this query idea for it 🙂

Implements first draft of quadtree lookup implemented via *BoundingBox*

068ec7d

Adds *SpatialEntity*, a new struct which implements *Spatial*

87b3c44

With this it is now possible to insert *Entities* from *World* into a quadtree And query for them using *BoundingBox* queries

Adds first draft of *SpatialRelation* System to populate a quadtree

81d23e0

and keep track of changes to entities within it

DerLando commented Jan 31, 2020

View reviewed changes

DerLando added 6 commits February 1, 2020 11:57

Starts refactoring of *Space* into its own resource

636b502

with a similar structure to the *NameTableBookkeeping*

Starts implementing *unimplemented* in *Space*

424fea0

Implements *Space.query_point()*, updates tests for *SpatialRelation*

b369bed

Removes test from *Space* As we do the testing inside of the *Spatial…

cae53ff

…Relation* system now

Implements *Space.query_region()*

9245876

Makes *clippy* happy

8d3e831

Preemptively circumvents newer versions of *euclid* being added in th…

879109d

…e future

DerLando marked this pull request as ready for review February 1, 2020 12:13

Merge branch 'master' into implement-quadtree-lookup

274f1a5

Michael-F-Bryan reviewed Feb 1, 2020

View reviewed changes

DerLando added 10 commits February 1, 2020 14:29

Fixes merge conflicts

29af58c

Fices merge conflict for *euclid* dependency in *Cargo.toml*

d69af7a

Reworks *query_point* and *query_region* methods of *space* into retu…

fffac42

…rnung *impl Iterator*

Updated tests for *Space*

db32c56

Makes arguments to *aabb_quadtree* constructor more explicit as const…

404e15f

…ants

Merged *Space.insert()* and *Space.modify()* into one method,

bd07cd7

added panics for unexpected outcomes

Changes *ReadStorage* of *SpatialRelation* to subscribe to *BoundingB…

d8597be

…ox* events

Merge branch 'master' of https://github.com/Michael-F-Bryan/arcs into…

1c2cb7d

… implement-quadtree-lookup

Added a tree constructor which takes a size to *Space*

3133b74

Added documentation for *Space*

be73c1e

DerLando added 4 commits February 3, 2020 21:51

Update Tests for *Space*, fixed bug in deleting of *SpatialEntity* fr…

c62794d

…om space when encountering *ComponentEvent::Removed*

Added *Resize* method to *Space* to allow insertion of items

0fa5f7d

out-of-bounds at a later stage

Merge branch 'master' of https://github.com/Michael-F-Bryan/arcs into…

76ca85c

… implement-quadtree-lookup

Fixed small un-needed .copy() inside of *Space.remove_by_id()*

0e44b3b

Michael-F-Bryan merged commit 44bf422 into Michael-F-Bryan:master Feb 4, 2020

Michael-F-Bryan mentioned this pull request Feb 4, 2020

Implement our own spatial lookup data structure #17

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements first draft of quadtree lookup implemented via BoundingBox #12

Implements first draft of quadtree lookup implemented via BoundingBox #12

DerLando commented Jan 30, 2020

Michael-F-Bryan commented Jan 31, 2020

DerLando commented Jan 31, 2020

DerLando commented Jan 31, 2020

DerLando Jan 31, 2020

DerLando commented Feb 1, 2020

Michael-F-Bryan commented Feb 1, 2020

DerLando commented Feb 1, 2020 •

edited

Loading

Michael-F-Bryan left a comment

Michael-F-Bryan Feb 1, 2020

DerLando Feb 1, 2020

DerLando Feb 2, 2020

Michael-F-Bryan Feb 2, 2020

DerLando Feb 3, 2020

Michael-F-Bryan Feb 1, 2020

DerLando Feb 2, 2020

Michael-F-Bryan Feb 1, 2020

DerLando Feb 1, 2020

DerLando Feb 3, 2020

Michael-F-Bryan Feb 1, 2020

DerLando Feb 2, 2020

Michael-F-Bryan Feb 3, 2020

DerLando Feb 3, 2020

Michael-F-Bryan Feb 1, 2020

DerLando Feb 1, 2020

Michael-F-Bryan Feb 1, 2020

DerLando Feb 1, 2020

DerLando Feb 1, 2020

DerLando commented Feb 3, 2020

Michael-F-Bryan commented Feb 4, 2020

Michael-F-Bryan commented Feb 4, 2020

DerLando commented Feb 4, 2020

Implements first draft of quadtree lookup implemented via *BoundingBox* #12

Implements first draft of quadtree lookup implemented via *BoundingBox* #12

Conversation

DerLando commented Jan 30, 2020

Michael-F-Bryan commented Jan 31, 2020

DerLando commented Jan 31, 2020

DerLando commented Jan 31, 2020

Choose a reason for hiding this comment

DerLando commented Feb 1, 2020

Michael-F-Bryan commented Feb 1, 2020

DerLando commented Feb 1, 2020 • edited Loading

Michael-F-Bryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerLando commented Feb 3, 2020

Michael-F-Bryan commented Feb 4, 2020

Michael-F-Bryan commented Feb 4, 2020

DerLando commented Feb 4, 2020

Implements first draft of quadtree lookup implemented via BoundingBox #12

Implements first draft of quadtree lookup implemented via BoundingBox #12

DerLando commented Feb 1, 2020 •

edited

Loading