WIP: Spatial Tree Accelerating Data Structures #783

JoaoDell · 2023-04-14T17:47:47Z

This PR introduces quadtree and octree data structures to FURY.

Below, an example of a quadtree generated and rendered with this module:

2023-04-14.14-37-41.mp4

And below, an example of an octree:

2023-04-14.14-41-09.mp4

The classes added contain the most basic implementation of those structures, and some simple functions for rendering them.

…ches.

…tions. Also improved printing of both trees with procedural printing of it sizes.

skoudoro

Hi @JoaoDell,

Thank you for this first version! Before the team starts any review, you will have to update your code to follow our projects standard. it means:

follow pep8 rules
python rules (snake_case for function for example)
check how we implement the test
look at numpy style guide standard. https://numpydoc.readthedocs.io/en/latest/format.html
we work with double quotes. Look at our codebase.
check if you really need a class for Point2D , Point3D, etc...? we have other structures or a dataclass do the job easily
simplify the implementation with basic python structure (dict, comprehension dict or list, etc...)
make sure that the test passed locally

filipinascimento

I have some specific comments, but @skoudoro already pointed out the problems regarding style and over engineering based on classes. (Points2D, etc).

Once you have a new version addressing the other issues I would be happy to review it again. I'm curious on the speed of this implementation, we can check this next time.

filipinascimento · 2023-04-14T18:18:07Z

fury/trees.py

+
+
+# QUADTREE IMPLEMENTATION
+class Point2d():


No need to have classes for Point2D and point 3D. we already use lists or numpy array interchangeably to store vectors. It is also faster because you can store several vectors in numpy.

I believe the problem is more of a confusion that the class name is generating. Point2D, Point3D are not just implementations of positions.

filipinascimento · 2023-04-14T18:27:56Z

fury/trees.py

+
+        return self._STR
+
+    def GetRoot(self):


Avoid methods starting with get or set unless they are really needed . We recommend to use properties or just the name of the property.

Ok, I will be looking into that. Is there any other place you think it didn't need to be used, or just that specific part?

Also, adhere to the Python naming convention used in FURY: remove_point() instead of RemovePoint().

JoaoDell · 2023-04-14T21:38:55Z

Hey @skoudoro and @filipinascimento, thanks for the quick reply, I will be looking into those issues soon! I just wanted to clarify better why I am using classes like Point2d and 3d to store coordinates so I can be sure they are really unecessary: Analyzing the whole code now maybe I left it redundant and abused of class engineering indeed, but I thought about using specific classes for that so a user that decides to use trees for any abstract purpose could build a class based on those and complement it with it's own necessary information for the intended purpose, still preserving the tree structure. Do you think that makes sense or a better approach could be taken? Maybe cleaning the base classes to make it simpler, or using only arrays, as you mentioned?

JoaoDell · 2023-04-14T23:18:39Z

Actually, about my last comment, after some thinking, I just realized that indeed it may be a redundant choice, as if someone wanted to do that, they would create a class using an array as a base, so you guys have a point, and I will try it later.

devmessias · 2023-04-17T23:15:42Z

"test_tree.py" is running without errors. But it seems more appropriate as an example that can be added following the standard of examples here: https://github.com/fury-gl/fury/tree/master/docs/examples.

…dditional functions and overloads into trees.py, and moved the last version of test_trees as an example into exmaples.

JoaoDell · 2023-04-21T14:42:17Z

Hey everyone, I am sorry for the big commit, I entered in a very rushed week and forgot to commit every time I got to a good checkpoint, but here it is. I've changed a lot of things regarding pep8 and python conventions in general, as well as implemented a test for the features and some other things.

skoudoro

Hi @JoaoDell,

Great! a lot of progress ! The style is much more readable.
Still some over-engineering to fix but we are getting there.

Point2D and Point3D class are not needed, they bring no value and more maintenance. Please remove them. Here what we are used to do:

# create a array of 2 points
my_point2d = np.array([[5, 1], [1,2]])
print(my_point2D.shape, my_point2D.ndim)
my_point3d = np.array([[5, 1, 0], [1,2, 1]])
print(my_point3D.shape, my_point3D.ndim)
# then you have access to all numpy and scipy library to play with your point. 
# No need to reimplement add, sub, etc....
# numpy and scipy have implemented already a lot and it is optimized.

Also, if you really want to have x, y , you can redefine numpy dtype.

I let you look at this, go deeper and update your code.

you can also, check Branch2D and Branch3D design. Do you really need a class ? maybe it is easier to maintain if you are a bit more functional style than OO style.

I let you look at all of that

JoaoDell · 2023-05-17T15:19:22Z

Hey everyone, just to let you know that i am currently working on the changes to this PR, I just haven't submitted yet because I am having issues with the testing file and implementation choices as i've abolished point2d and 3d, so you can expect an update soon

JoaoDell · 2023-05-23T20:01:28Z

Hello everyone, so I just commited my lastest attempt on adressing the points you mentioned. I have abolished point2d and point3d usage for simple arrays, and added an id system based on python native uuid to keep track of the points inside the tree. I must admit I am not that sure about all the choices I made to make this work, but it is working and it seems simple to use, so I guess I can accept that as a checkpoint.

skoudoro · 2023-06-07T20:18:04Z

@devmessias @filipinascimento @guaje. Can you review this PR ?

filipinascimento

Thanks for the PR. I've found that the API is still complicated to be used by a regular user.
This can be simplified by exposing the add_points/remove_points functionality directly in the respective Tree objects. With that, users would need to provide just the set of points (and indices) during the creation of the Tree objects, or afterwards via add/remove methods.

You can also implement some useful methods that uses the tree structure, for instance to get the k closest points to a given point, closest point/points to a line (ray), both in 2D and 3D. The method can also be used to get the closest points up to a certain given distance. This is extremely useful for certain types of interactions.

Regarding the demos/examples, they need to have explanation of what is happening. I would suggest 3 examples:

One for just drawing the quad or octree for a set of random points.
Another for showcasing scalability, increasing the number of nodes progressively and seeing the quad or octree dynamically accounting for the new points.
Finally, the last example should be an useful one. In this case it could be a ball that can be moved or is animated with a predefined path. Then you show the closest 3 or 4 points to that ball as the ball moves (you can use lines to connect them to the closest points, or simply highlight them.
I recommend to also check for scalability. How fast isyour approach as you increase the number of points?

filipinascimento · 2023-06-23T20:02:55Z

docs/examples/trees_example.py

+from fury import window, actor, lib
+import numpy as np
+
+


Please, check the other examples. You need to include some documentation for the example. For instance, explaining in details what is happening in the code, why you are doing it and how to do it.

filipinascimento · 2023-06-23T20:06:09Z

docs/examples/trees_example.py

+
+
+# TESTING A QUADTREE OF RANDOM 2D POINTS
+br = trees.Branch2d(4, 0.0, 1.0, 0.0, 1.0)


We may need to add a functionality in which the users do not need to deal with branches or add points, etc. You can keep this functionality as advanced usage. But the common usage would be just
Tree2D(points)
tree.addPoints()
maybe something to delete points as well or to update points with a new array.

filipinascimento · 2023-06-23T20:07:15Z

docs/examples/trees_example.py

+    reset_camera=True,
+    order_transparent=True)
+
+scene.add(actorPoints)


It would be great if all the tree structures would appear in the same screen instead of creating new scenes for each of them separately. If you use a grid of plots, with them side by side, maybe rotating along their axes.

filipinascimento · 2023-06-23T20:12:29Z

docs/examples/trees_example.py

+
+actorslist = trees.actor_from_branch_2d(tree.root(), (1.0, 1.0, 1.0), 1.0)
+zeros = np.zeros((npoints, 1))
+actorPoints = actor.dots(np.hstack((tree.root().all_points_list(), zeros)), (1.0, 0.5, 0.4), 1, 5)


Please, don't use the dots api. You can try to use billboards (with spheres) or spheres actor instead.

filipinascimento · 2023-06-23T20:13:22Z

fury/trees.py

+        return cubeActor
+
+
+def actor_from_branch_3d(branch: Branch3d, color=(


You can also leave an option to show the points in that branch.

JoaoDell and others added 9 commits April 12, 2023 16:45

First tree data structures integration.

cec2afc

Added a testing program for the trees and a new function for the bran…

7378a7d

…ches.

Minor change to the trees test program and octree

b143cac

Reimplemented the octree rendering function with fury's existing func…

61f7f2f

…tions. Also improved printing of both trees with procedural printing of it sizes.

Removed local path import

bf446e7

Added a 2d rendering for quadtrees and improved exception handling

7a3ca09

Merge branch 'trees' of https://github.com/JoaoDell/fury into trees

92fa925

Improved exception handling and added rendering for quadtrees

b5db0ff

Merge remote-tracking branch 'fury-gl/master' into trees

b6ee049

skoudoro requested changes Apr 14, 2023

View reviewed changes

filipinascimento reviewed Apr 14, 2023

View reviewed changes

Changed test_trees.py to a pytest testing program, implemented some a…

acaca5c

…dditional functions and overloads into trees.py, and moved the last version of test_trees as an example into exmaples.

skoudoro requested changes Apr 21, 2023

View reviewed changes

JoaoDell changed the title ~~Spatial Tree Accelerating Data Structures~~ WIP: Spatial Tree Accelerating Data Structures May 17, 2023

JoaoDell and others added 3 commits May 23, 2023 16:33

Merge branch 'fury-gl:master' into trees

8e944c3

Point storing and tracking overhaul

ffe4788

Merge branch 'trees' of https://github.com/JoaoDell/fury into trees

d4cf462

Corrected code spell issues

ad5c4d9

filipinascimento reviewed Jun 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Spatial Tree Accelerating Data Structures #783

WIP: Spatial Tree Accelerating Data Structures #783

JoaoDell commented Apr 14, 2023

skoudoro left a comment •

edited

Loading

filipinascimento left a comment •

edited

Loading

filipinascimento Apr 14, 2023

devmessias Apr 17, 2023

filipinascimento Apr 14, 2023

JoaoDell Apr 14, 2023

guaje Apr 14, 2023

JoaoDell commented Apr 14, 2023

JoaoDell commented Apr 14, 2023 •

edited

Loading

devmessias commented Apr 17, 2023

JoaoDell commented Apr 21, 2023

skoudoro left a comment •

edited

Loading

JoaoDell commented May 17, 2023

JoaoDell commented May 23, 2023

skoudoro commented Jun 7, 2023

filipinascimento left a comment

filipinascimento Jun 23, 2023

filipinascimento Jun 23, 2023

filipinascimento Jun 23, 2023

filipinascimento Jun 23, 2023

filipinascimento Jun 23, 2023



		# TESTING A QUADTREE OF RANDOM 2D POINTS
		br = trees.Branch2d(4, 0.0, 1.0, 0.0, 1.0)

		return cubeActor


		def actor_from_branch_3d(branch: Branch3d, color=(

WIP: Spatial Tree Accelerating Data Structures #783

Are you sure you want to change the base?

WIP: Spatial Tree Accelerating Data Structures #783

Conversation

JoaoDell commented Apr 14, 2023

skoudoro left a comment • edited Loading

Choose a reason for hiding this comment

filipinascimento left a comment • edited Loading

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

JoaoDell commented Apr 14, 2023

JoaoDell commented Apr 14, 2023 • edited Loading

devmessias commented Apr 17, 2023

JoaoDell commented Apr 21, 2023

skoudoro left a comment • edited Loading

Choose a reason for hiding this comment

JoaoDell commented May 17, 2023

JoaoDell commented May 23, 2023

skoudoro commented Jun 7, 2023

filipinascimento 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

skoudoro left a comment •

edited

Loading

filipinascimento left a comment •

edited

Loading

JoaoDell commented Apr 14, 2023 •

edited

Loading

skoudoro left a comment •

edited

Loading