NF: Sphere, Geometry & Material #946

m-agour · 2024-12-07T03:04:36Z

Added Sphere actor.

pep8speaks · 2024-12-07T03:04:44Z

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

In the file fury/actor.py:

Line 95:80: E501 line too long (81 > 79 characters)

In the file fury/material.py:

Line 5:80: E501 line too long (84 > 79 characters)

In the file fury/utils.py:

Line 29:80: E501 line too long (81 > 79 characters)
Line 154:80: E501 line too long (84 > 79 characters)

Comment last updated at 2024-12-16 15:56:33 UTC

m-agour · 2024-12-07T03:09:02Z

I will fix the pep8 and upload the tests

skoudoro

Hi @m-agour,

thank you for this. Do you know when this will be ready?

Let's try to move forward !

skoudoro · 2024-12-09T21:17:50Z

fury/actor/__init__.py

@@ -0,0 +1 @@
+from .actor import sphere


please, use absolute import

skoudoro · 2024-12-09T21:18:08Z

fury/actor/actor.py

+import fury.primitive as fp
+import numpy as np
+from fury.v2.actor.materials import _create_mesh_material
+from fury.v2.actor.geometry import buffer_to_geometry, create_mesh


import order incorrect,

skoudoro · 2024-12-09T21:18:20Z

fury/actor/actor.py

+    opacity=1,
+    material='phong',
+    enable_picking=True
+):


do not forget docstring

skoudoro · 2024-12-09T21:19:10Z

fury/actor/actor.py

+    if faces is None and vertices is None:
+        vertices, faces = fp.prim_sphere(phi=phi, theta=theta)


what happens if only vertices is None ? will it work?

what happens if only vertices is None ? will it work?

@skoudoro No, It will not work and I can't imagine any use case for giving the vertices and faces of a sphere to create a sphere. Do you think we should remove it or keep it?

skoudoro · 2024-12-09T21:19:59Z

fury/actor/actor.py

+        scales=scales,
+    )
+    big_verts, big_faces, big_colors, _ = res
+    print(big_colors)


print to remove

skoudoro · 2024-12-09T21:21:37Z

fury/actor/geometry.py

@@ -0,0 +1,32 @@
+from pygfx import Geometry, Mesh


not sure geometry should be under actor folder

skoudoro · 2024-12-09T21:22:11Z

fury/actor/actor.py

@@ -0,0 +1,54 @@
+import fury.primitive as fp


not sure actor should be under actor folder. let's keep it simple and create submodule later

@m-agour and @skoudoro The broader picture on this is, it should have actor directory and actors should be divided in files such as sphere and line should be part of base.py and odf and slicer should be part of medical.py segregating the actors based on buckets or types.

maharshi-gor

Please take a look at requesting changes and move faster on this @m-agour .

Thanks!

maharshi-gor · 2024-12-12T09:18:16Z

fury/actor/actor.py

@@ -0,0 +1,54 @@
+import fury.primitive as fp


@m-agour and @skoudoro The broader picture on this is, it should have actor directory and actors should be divided in files such as sphere and line should be part of base.py and odf and slicer should be part of medical.py segregating the actors based on buckets or types.

maharshi-gor · 2024-12-12T09:19:50Z

fury/actor/materials.py

+            pick_write=enable_picking,
+            color_mode='vertex' if color is None else 'auto',
+            color=color if color is not None else (1, 1, 1, opacity if color is None else 1),
+        )


Throw an exception if the material does not equal to one of the configured types.

skoudoro · 2024-12-12T15:01:36Z

The broader picture on this is, it should have actor directory and actors should be divided in files such as sphere and line should be part of base.py and

I think it is over-engineering @maharshi-gor. Let's keep it simple and split when we feel the need.

odf and slicer should be part of medical.py segregating the actors based on buckets or types.

odf can be used in other field like Geophysics, Astrophysics and Cosmology. We want fury to be generic. if odf is in medical.py, other field might avoid this file for no reason @maharshi-gor

skoudoro · 2024-12-13T05:39:58Z

Also, try to squash your commit @m-agour please!

m-agour · 2024-12-13T05:45:11Z

Thank you @skoudoro @maharshi-gor for your reviews I am already working on them and would finish them by tomorrow the 14th.

m-agour · 2024-12-13T05:46:02Z

Also, try to squash your commit @m-agour please!

Will do @skoudoro

NF: mesh material NF: geometry & mesh NF: added sphere actor RF: init actors BF: fix `sphere` opacity RF: pep8 issues fix & absolute import RF: convert `actor` module into fury/actor.py DOC: updated Sphere&material docstring

skoudoro

Hi @m-agour,

Thank you for the update. Here some initial review before trying it.

Also, Can you run pre-commit to make to follow the standard?

pip install pre-commit
pre-commit install
pre-commit run my_py_files

skoudoro · 2024-12-15T20:48:44Z

fury/actor.py

+        Spheres positions.
+    colors : ndarray, shape (N, 3) or (N, 4) or tuple (3,) or tuple (4,)
+        RGB or RGBA (for opacity) R, G, B, and A should be in the range [0, 1].
+    radii : float or ndarray, shape (N,)


, optional to add

you just missed this one

fury/actor.py

fury/material.py

skoudoro

Ok, overall, it looks good!

Thank you for this @m-agour. I need to try it locally, butI think it is ready to be merged.

@maharshi-gor, Do you want to give it a look? if no news, I will go ahead and merge it tomorrow.

maharshi-gor · 2024-12-16T19:49:26Z

I haven't tried as I need to device the tutorial on this. But looking at the PR LGTM.

I will give feedback as I have those tutorials ready.

m-agour · 2024-12-16T20:26:09Z

Thank you, @skoudoro and @maharshi-gor for your reviews!

skoudoro requested changes Dec 9, 2024

View reviewed changes

maharshi-gor reviewed Dec 12, 2024

View reviewed changes

m-agour force-pushed the sphere-&-material branch from 6b40a10 to 4e15a30 Compare December 13, 2024 05:13

m-agour force-pushed the sphere-&-material branch from 768a0af to a05fe70 Compare December 15, 2024 06:31

m-agour marked this pull request as ready for review December 15, 2024 06:32

NF: add sphere actor w/ materials

d4c7660

NF: mesh material NF: geometry & mesh NF: added sphere actor RF: init actors BF: fix `sphere` opacity RF: pep8 issues fix & absolute import RF: convert `actor` module into fury/actor.py DOC: updated Sphere&material docstring

m-agour force-pushed the sphere-&-material branch from a05fe70 to d4c7660 Compare December 15, 2024 06:41

skoudoro reviewed Dec 15, 2024

View reviewed changes

m-agour added 2 commits December 16, 2024 10:53

DOC: addressed docs review from @skoudoro

e76d449

RF: restructured the _create_mesh_material function

8b0463c

skoudoro reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NF: Sphere, Geometry & Material #946

NF: Sphere, Geometry & Material #946

m-agour commented Dec 7, 2024

pep8speaks commented Dec 7, 2024 •

edited

Loading

m-agour commented Dec 7, 2024

skoudoro left a comment

skoudoro Dec 9, 2024

skoudoro Dec 9, 2024

skoudoro Dec 9, 2024

skoudoro Dec 9, 2024

m-agour Dec 14, 2024

skoudoro Dec 9, 2024

skoudoro Dec 9, 2024

skoudoro Dec 9, 2024

maharshi-gor Dec 12, 2024

maharshi-gor left a comment

maharshi-gor Dec 12, 2024

maharshi-gor Dec 12, 2024

skoudoro commented Dec 12, 2024

skoudoro commented Dec 13, 2024

m-agour commented Dec 13, 2024

m-agour commented Dec 13, 2024

skoudoro left a comment

skoudoro Dec 15, 2024

skoudoro Dec 16, 2024

skoudoro left a comment

maharshi-gor commented Dec 16, 2024

m-agour commented Dec 16, 2024

		if faces is None and vertices is None:
		vertices, faces = fp.prim_sphere(phi=phi, theta=theta)

NF: Sphere, Geometry & Material #946

Are you sure you want to change the base?

NF: Sphere, Geometry & Material #946

Conversation

m-agour commented Dec 7, 2024

pep8speaks commented Dec 7, 2024 • edited Loading

Comment last updated at 2024-12-16 15:56:33 UTC

m-agour commented Dec 7, 2024

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

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

maharshi-gor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoudoro commented Dec 12, 2024

skoudoro commented Dec 13, 2024

m-agour commented Dec 13, 2024

m-agour commented Dec 13, 2024

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

skoudoro left a comment

Choose a reason for hiding this comment

maharshi-gor commented Dec 16, 2024

m-agour commented Dec 16, 2024

pep8speaks commented Dec 7, 2024 •

edited

Loading