Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implements first draft of quadtree lookup implemented via *BoundingBox* #12
Implements first draft of quadtree lookup implemented via *BoundingBox* #12
Changes from 11 commits
068ec7d
87b3c44
81d23e0
636b502
424fea0
b369bed
cae53ff
9245876
8d3e831
879109d
274f1a5
29af58c
d69af7a
fffac42
db32c56
404e15f
bd07cd7
d8597be
1c2cb7d
3133b74
be73c1e
c62794d
0fa5f7d
76ca85c
0e44b3b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:Quadtree
implementationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some more thinking on this:
Space
needs to implementDefault
so we can use it as aResource
, this inevitely means we have to define some default starting parameters, as we can not supply arguments toSpace::default()
.aabb_quadtree.insert()
which detects objects outside the bounds of the tree-root and resize the tree if neededSpatialEntity
fits inThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add
Space
as a resource in the bookkeeping system'ssetup()
method then useReadExpect
when trying to access it?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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an unnecessary mix of two unrelated systems. So I'd rather stick to
Space
being it's own systemIt really looks like we need a better backend for our
Space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably pull these out into constants, because
true, 4, 16, 8, 4
doesn't really mean anything.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using
id()
here instead of the fullEntity
? AnEntity
also contains aGeneration
so you can't get the equivalent of a use-after-free when an entity gets deleted andspecs
allocates the sameIndex
to an entity created later on.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually that makes more sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a thought, but could we merge
modify()
andinsert()
by using the entry API?Also, should it be an error to modify a
SpatialEntity
which doesn't already exist inself.ids
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind this is that we can't insert the same entity twice? If it already exists we modify it instead?
If we keep two different methods I would agree on throwing an error when trying to modify a
SpatialEntity
which is not already present.It would be less explicit but probably more usage-friendly to merge those two methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged the two methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Should this accept a
radius
parameter instead of assuming the user always wantsQUERY_POINT_RADIUS
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's even more complicated then that:
This should accept a
radius
inDrawingUnit
so the selection size never changes regardless of the * Zoom-Factor*.I don't have a good idea at the moment where to wrap this functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the underlying quad tree provide an API for searching lazily (i.e. by returning an iterator) instead of greedily collecting into a
Vec
?From my experience, a lot of the time you're only interested in the first result matching a predicate so it's a waste of time/memory to populate a vector with all objects within the region. Returning an
impl Iterator<Item = SpatialEntity>
would also feel a bit more idiomatic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the query function is hardcoded to return a
small-vec
, maybe it would be possible using thecustom_query
method, I can look into that.Also +1 on
impl Iterator<Item = SpatialEntity>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed return type to
impl Iterator<Item = SpatialEntity>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
self.quadtree
have aclear()
method so we can reuse any underlying allocations?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it has not, which is why I opted for this approach.
Is there a more efficient way?