Added primitives count to the the Actor's polydata #617

m-agour · 2022-07-01T15:36:33Z

As @filipinascimento, @guaje, and I was trying to find an approach to only apply the keyframe animation on a partial subset of the actor's primitives not all of them, we found that there is no currently implemented way to get the primitives count of a single actor.
In this PR, the primitive's count is saved inside the polydata of the actor.
This can be used to distinguish the vertices of different primitives

… the primitives count

filipinascimento

This PR is simple, but really important to anything that requires changing or interacting with individual elements of a multi entity actor.

It looks good to me. The tests are failing for other reasons.

Maybe for another PR, it would be interesting to have some auxiliary functions to directly access the memory view of positions, colors and other attributes of the independent entities.

filipinascimento · 2022-07-04T02:26:09Z

fury/utils.py

@@ -690,7 +719,8 @@ def get_actor_from_primitive(vertices, triangles, colors=None,
        culling of polygons based on orientation of normal with respect to
        camera. If backface culling is True, polygons facing away from camera
        are not drawn. Default: True
-
+    prim_count: bool
+        primitives count inside the actor


maybe something like: primitives count to be associated with the actor

m-agour · 2022-07-04T02:54:48Z

This PR is simple, but really important to anything that requires changing or interacting with individual elements of a multi entity actor.

It looks good to me. The tests are failing for other reasons.

Maybe for another PR, it would be interesting to have some auxiliary functions to directly access the memory view of positions, colors and other attributes of the independent entities.

Thanks, @filipinascimento for your review. After today's meeting, I will check if the error is caused by this PR or something else.
Yes, the plan is to use what's done in this PR to do some operations on a single actor's primitives on both the GPU side (vertex shader maybe with the help of vertex-index attribute (not sure if this is a good solution)) and the CPU side (memory views as you mentioned).

ganimtron-10

Hi @m-agour ,
Good Work. All the methods were well documented.
I have mentioned a few comments, PTAL.
Thanks!

ganimtron-10 · 2022-07-05T05:34:40Z

fury/utils.py

@@ -690,7 +719,8 @@ def get_actor_from_primitive(vertices, triangles, colors=None,
        culling of polygons based on orientation of normal with respect to
        camera. If backface culling is True, polygons facing away from camera
        are not drawn. Default: True
-
+    prim_count: int


,optional

ganimtron-10 · 2022-07-05T05:36:11Z

fury/tests/test_actors.py

+    centers = np.array([[1, 1, 1], [2, 2, 2]])
+    box_actor = actor.box(centers)
+    npt.assert_equal(get_primitives_count_from_actor(box_actor), len(centers))
+    box_actor = actor.sphere(centers, (1, 0, 0))


Maybe sphere_actor or sphere

ganimtron-10 · 2022-07-05T05:36:56Z

fury/tests/test_actors.py

+    npt.assert_equal(get_primitives_count_from_actor(box_actor), len(centers))
+    box_actor = actor.sphere(centers, (1, 0, 0))
+    npt.assert_equal(get_primitives_count_from_actor(box_actor), len(centers))
+    box_actor = actor.sphere(centers, (1, 0, 0), use_primitive=True)


Same as above
or above name with prim_ prefix to differentiate the actors

Thanks, @ganimtron-10 for your review! Helpful as always.

This reverts commit d940fe6.

guaje

@m-agour This is a much-needed feature. Thanks for this PR.

I have added some comments, see below.

fury/tests/test_actors.py

guaje · 2022-07-05T21:38:52Z

fury/utils.py

@@ -715,6 +745,7 @@ def get_actor_from_primitive(vertices, triangles, colors=None,

    current_actor = get_actor_from_polydata(pd)
    current_actor.GetProperty().SetBackfaceCulling(backface_culling)
+    add_primitives_count_to_actor(current_actor, prim_count)


You might see in the utils.py file that we have both the get/set_polydata_property (e.g., get_polydata_normals and set_polydata_normals) and the property_from/to_actor (e.g., normals_from_actor and normals_to_actor). The reason for this is that the polydata functions are meant to be used for tasks like actor creations since at some point you'll have access to the polydata before it goes to the mapper. While the actor functions are for specific use cases, they generally require getting the mapper to get the polydata to get the desired attribute finally. This process is inefficient. Therefore, I would recommend adding the respective get/set_polydata_primitives_count and using them here since you can access the polydata here.

Hello @guaje, thanks for your review. I made all the changes you requested I believe.
I also noticed this PR was not applicable on some actors while doing the tests. so I had them added.
I hope I didn't miss anything.

Yeah, that has happened on several occasions. That was the main reason I asked you to test all the supported actors. In the past, we have assumed that some functions are supposed to work equally on all the actors, yet unfortunately, that's not the case since we have different methods to create them.

fury/utils.py

…ives

pep8speaks · 2022-07-06T17:02:22Z

Hello @m-agour! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-15 23:25:36 UTC

… actors

codecov · 2022-07-06T18:59:10Z

Codecov Report

Merging #617 (4907bd2) into master (63b87a8) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

❗ Current head 4907bd2 differs from pull request most recent head f17f880. Consider uploading reports for the commit f17f880 to get more accurate results

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
- Coverage   51.84%   51.76%   -0.09%     
==========================================
  Files         105      105              
  Lines       22337    22941     +604     
  Branches     2460     2532      +72     
==========================================
+ Hits        11581    11875     +294     
- Misses      10381    10685     +304     
- Partials      375      381       +6

Impacted Files	Coverage Δ
fury/actor.py	`0.00% <0.00%> (ø)`
fury/ui/tests/test_elements_callback.py	`0.00% <0.00%> (ø)`
fury/utils.py	`0.00% <0.00%> (ø)`
fury/window.py	`0.00% <0.00%> (ø)`
fury/fury/tests/test_window.py	`86.37% <0.00%> (-7.36%)`	⬇️
fury/fury/data/fetcher.py	`50.42% <0.00%> (-0.92%)`	⬇️
fury/decorators.py	`0.00% <0.00%> (ø)`
fury/ui/elements.py	`0.00% <0.00%> (ø)`
fury/fury/decorators.py	`100.00% <0.00%> (ø)`
fury/ui/tests/test_elements.py	`0.00% <0.00%> (ø)`
... and 8 more

guaje

@m-agour Thanks for adding the missing tests.

I have added the following comments based on the latest update. Additionally, I noticed you added primitives_count support to the square actor but there is no test for it. Last, but not least important, take a look at this test. It could help you to improve your tests.

fury/tests/test_utils.py

fury/utils.py

fury/tests/test_actors.py

guaje · 2022-07-07T17:52:13Z

fury/tests/test_actors.py

+    sphere_actor = actor.sphere(centers, colors)
+    npt.assert_equal(get_actor_primitives_count(sphere_actor), len(centers))
+
+    prim_sphere_actor = actor.sphere(centers, colors, use_primitive=False)


Isn't use_primitive = False by default?

Yes, but I thought this might get changed at any time (use_primitive=True by default). so it's better to explicitly set it to keep the tests valid.

fury/tests/test_actors.py

… pep8 issues and added

m-agour · 2022-07-07T22:16:08Z

@m-agour Thanks for adding the missing tests.

I have added the following comments based on the latest update. Additionally, I noticed you added primitives_count support to the square actor but there is no test for it. Last, but not least important, take a look at this test. It could help you to improve your tests.

Hi, @guaje, thanks for rereviewing this PR, I made some of the changes and yes you are right this test can use some optimization.
I will restructure it tomorrow to use a for loop instead.

skoudoro · 2022-07-12T16:45:07Z

Hi @m-agour,

What is the status of this PR?

m-agour · 2022-07-13T00:35:42Z

Hi @m-agour,

What is the status of this PR?

Hi @skoudoro ,
I think it needs a final review after my last commits.

guaje

LGTM. Just some renaming for 2 functions left.

fury/tests/test_utils.py

fury/tests/test_actors.py

guaje · 2022-07-15T18:51:28Z

LGTM. If there are no more comments, I'll merge this PR.

Anything else to add @filipinascimento, @skoudoro, @xtanion, @ganimtron-10?

skoudoro

Hi @m-agour,

it looks great! Apart from the small comments below, +1 for merging.

Thank you for this great review also @guaje. After tomorrow, Feel free to merge it.

skoudoro · 2022-07-15T20:15:02Z

fury/tests/test_actors.py

+        [actor.cone, {**args_3, 'use_primitive': True}, cen_c],
+        [actor.arrow, {**args_3, 'repeat_primitive': False}, cen_c],
+        [actor.arrow, {**args_3, 'repeat_primitive': True}, cen_c],
+        [actor.dots, {'points': centers}, cen_c],


actor.dot instead

the plural is deprecated.

guaje · 2022-07-16T16:11:20Z

@m-agour Good job! Merging.

m-agour added 4 commits July 1, 2022 16:46

Added a getter and a setter for the primitives count.

9182972

assigning the primitive count while creating the actor

e440604

updated the actors in both repeat_source and repeat_primitives to set…

d288f23

… the primitives count

Added tests

edd9bbf

skoudoro requested review from guaje, filipinascimento, xtanion and ganimtron-10 July 3, 2022 23:03

filipinascimento approved these changes Jul 4, 2022

View reviewed changes

filipinascimento suggested changes Jul 4, 2022

View reviewed changes

fixed the description and the datatype

f1ec0a5

skoudoro added the type:Enhancement label Jul 4, 2022

ganimtron-10 reviewed Jul 5, 2022

View reviewed changes

m-agour added 2 commits July 5, 2022 22:40

Fixed vars names.

d940fe6

Revert "Fixed vars names."

319a675

This reverts commit d940fe6.

guaje requested changes Jul 5, 2022

View reviewed changes

m-agour added 2 commits July 6, 2022 19:01

Added tests for other actors

2e29d70

Updated forgotten actors not using the repeat_source or repeat_primit…

a80fcea

…ives

m-agour added 9 commits July 6, 2022 19:57

Added get/set_polydata_primitives_count and updated tests

0be3bce

changed the functions names

f120a13

PIP issue

a2f1503

Using set_polydata_primitives_count instead of set_actor_...

998b80d

Another pep issue

428b5a9

Added compound test

1f9b658

Added streamtube test

8d3a283

Set primitives count to polydata directly in dot, line and streamtube…

75a2e2f

… actors

was comparing to the wrong length

15fd506

m-agour requested a review from guaje July 6, 2022 18:36

guaje requested changes Jul 7, 2022

View reviewed changes

m-agour added 3 commits July 7, 2022 23:30

Renamed two functions for the naming consistency

95a4f8c

Added square test, removed duplicated test and unused imports, solved…

95bbac6

… pep8 issues and added

added lines array at top

8c746b5

restructured the test

4907bd2

guaje reviewed Jul 14, 2022

View reviewed changes

fury/tests/test_utils.py Outdated Show resolved Hide resolved

fury/tests/test_utils.py Outdated Show resolved Hide resolved

fury/tests/test_actors.py Outdated Show resolved Hide resolved

Renamed two tests

59787dd

skoudoro approved these changes Jul 15, 2022

View reviewed changes

Should be actor.dot

f17f880

guaje merged commit b89e526 into fury-gl:master Jul 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added primitives count to the the Actor's polydata #617

Added primitives count to the the Actor's polydata #617

m-agour commented Jul 1, 2022 •

edited

filipinascimento left a comment •

edited

filipinascimento Jul 4, 2022

m-agour commented Jul 4, 2022

ganimtron-10 left a comment •

edited

ganimtron-10 Jul 5, 2022

ganimtron-10 Jul 5, 2022

ganimtron-10 Jul 5, 2022

m-agour Jul 5, 2022

guaje left a comment

guaje Jul 5, 2022

m-agour Jul 6, 2022

guaje Jul 7, 2022

pep8speaks commented Jul 6, 2022 •

edited

codecov bot commented Jul 6, 2022 •

edited

guaje left a comment

guaje Jul 7, 2022

m-agour Jul 7, 2022

m-agour commented Jul 7, 2022

skoudoro commented Jul 12, 2022

m-agour commented Jul 13, 2022

guaje left a comment

guaje commented Jul 15, 2022

skoudoro left a comment

skoudoro Jul 15, 2022

skoudoro Jul 15, 2022

guaje commented Jul 16, 2022

Added primitives count to the the Actor's polydata #617

Added primitives count to the the Actor's polydata #617

Conversation

m-agour commented Jul 1, 2022 • edited

filipinascimento left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-agour commented Jul 4, 2022

ganimtron-10 left a comment • edited

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

guaje 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

pep8speaks commented Jul 6, 2022 • edited

Comment last updated at 2022-07-15 23:25:36 UTC

codecov bot commented Jul 6, 2022 • edited

Codecov Report

guaje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-agour commented Jul 7, 2022

skoudoro commented Jul 12, 2022

m-agour commented Jul 13, 2022

guaje left a comment

Choose a reason for hiding this comment

guaje commented Jul 15, 2022

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guaje commented Jul 16, 2022

m-agour commented Jul 1, 2022 •

edited

filipinascimento left a comment •

edited

ganimtron-10 left a comment •

edited

pep8speaks commented Jul 6, 2022 •

edited

codecov bot commented Jul 6, 2022 •

edited