Skip to content
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

Fixed #550 : Added necessary allignment between glyph creation and ac… #551

Merged
merged 15 commits into from
Apr 4, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 77 additions & 5 deletions fury/actor.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
PolyDataNormals, Assembly, LODActor, VTK_UNSIGNED_CHAR,
PolyDataMapper2D, ScalarBarActor, PolyVertex, CellArray,
UnstructuredGrid, DataSetMapper, ConeSource, ArrowSource,
SphereSource, CylinderSource, TexturedSphereSource,
SphereSource, CylinderSource, DiskSource, TexturedSphereSource,
Texture, FloatArray, VTK_TEXT_LEFT, VTK_TEXT_RIGHT,
VTK_TEXT_BOTTOM, VTK_TEXT_TOP, VTK_TEXT_CENTERED,
TexturedActor2D, TextureMapToPlane, TextActor3D,
Expand Down Expand Up @@ -1609,21 +1609,93 @@ def cylinder(centers, directions, colors, radius=0.05, heights=1,
>>> # window.show(scene)

"""
src = CylinderSource() if faces is None else None

if src is not None:
Comment on lines -1612 to -1614
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to keep this. It allows using your own faces and vertices if you build yourself a cylinder.

it give us flexibility for future change in the API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be from an early commit, before I addressed Guaje's comments. In the more recent commits, values for src and rotate are appropriately set to None if faces is not None.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point which commit?

I do not find it on master as you can see here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you revert it? thank you

Copy link
Contributor Author

@Sassafrass6 Sassafrass6 Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m looking on the glyph_orientation branch of my GitHub’s fury fork, here.

The link you’ve posted is from fury_gl:master, right?
There is no orientation argument, no disk function, etc.

What are you referencing, when you ask me to “revert it”?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is fury-gl:master. If I merge your PR as it is, this modification will be applied which we do not want.

So either:

  • you rebase your PR
  • you merge master in your branch.
  • you just change this part of the code yourself and commit it.

Sorry for not being explicit, this is what I meant by reverting it (keep this feature already in master).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I review the history of this, no need to do any changes, I will go ahead and merge it. Thanks @Sassafrass6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean now.
15 commits is a lot to review. Thank you for looking through it all.

if faces is None:
src = CylinderSource()
src.SetCapping(capped)
src.SetResolution(resolution)
src.SetRadius(radius)
rotate = np.array([[0, 1, 0, 0],
[-1, 0, 0, 0],
[0, 0, 1, 0],
[0, 0, 0, 1]])
else:
src = None
rotate = None

cylinder_actor = repeat_sources(centers=centers, colors=colors,
directions=directions,
active_scalars=heights, source=src,
vertices=vertices, faces=faces)
vertices=vertices, faces=faces,
orientation=rotate)

return cylinder_actor


def disk(centers, directions, colors, rinner=0.3,
Sassafrass6 marked this conversation as resolved.
Show resolved Hide resolved
router=0.7, cresolution=6, rresolution=2, vertices=None, faces=None):
"""Visualize one or many disks with different features.

Parameters
----------
centers : ndarray, shape (N, 3)
Disk positions
directions : ndarray, shape (N, 3)
The orientation vector of the disk.
colors : ndarray (N,3) or (N, 4) or tuple (3,) or tuple (4,)
RGB or RGBA (for opacity) R, G, B and A should be at the range [0, 1]
rinner : float
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float, optional instead

disk inner radius, default: 0.3
router : float
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float, optional instead

disk outer radius, default: 0.5
cresolution: int, default: 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int, optional instead

Number of facets used to define perimeter of disk.
rresolution: int, default: 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int, optional instead

Number of facets used radially.
vertices : ndarray, shape (N, 3)
The point cloud defining the disk.
faces : ndarray, shape (M, 3)
If faces is None then a disk is created based on theta and phi angles
If not then a disk is created with the provided vertices and faces.

Returns
-------
disk_actor: Actor

Examples
--------
>>> from fury import window, actor
>>> import numpy as np
>>> scene = window.Scene()
>>> centers = np.random.rand(5, 3)
>>> dirs = np.random.rand(5, 3)
>>> colors = np.random.rand(5, 4)
>>> actor = actor.disk(centers, dirs, colors,
>>> rinner=.1, router=.8, cresolution=30)
>>> scene.add(actor)
>>> window.show(scene)
"""
if faces is None:
src = DiskSource()
src.SetCircumferentialResolution(cresolution)
src.SetRadialResolution(rresolution)
src.SetInnerRadius(rinner)
src.SetOuterRadius(router)
rotate = np.array([[0, 0, -1, 0],
[0, 1, 0, 0],
[1, 0, 0, 0],
[0, 0, 0, 1]])
else:
src = None
rotate = None

disk_actor = repeat_sources(centers=centers, colors=colors,
directions=directions, source=src,
vertices=vertices, faces=None,
Sassafrass6 marked this conversation as resolved.
Show resolved Hide resolved
orientation=rotate)

return disk_actor


def square(centers, directions=(1, 0, 0), colors=(1, 0, 0), scales=1):
"""Visualize one or many squares with different features.

Expand Down
1 change: 1 addition & 0 deletions fury/lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@
##############################################################
# vtkFiltersGeneral Module
SplineFilter = fgvtk.vtkSplineFilter
TransformPolyDataFilter = fgvtk.vtkTransformPolyDataFilter

##############################################################
# vtkFiltersHybrid Module
Expand Down
24 changes: 17 additions & 7 deletions fury/tests/test_actors.py
Original file line number Diff line number Diff line change
Expand Up @@ -919,20 +919,26 @@ def test_basic_geometry_actor(interactive=False):

def test_advanced_geometry_actor(interactive=False):
xyz = np.array([[0, 0, 0], [50, 0, 0], [100, 0, 0]])
dirs = np.array([[0, 1, 0], [1, 0, 0], [0, 0.5, 0.5]])
dirs = np.array([[0.5, 0.5, 0.5], [0.5, 0, 0.5], [0, 0.5, 0.5]])

actor_list = [[actor.cone, {'directions': dirs, 'resolution': 8}],
[actor.arrow, {'directions': dirs, 'resolution': 9}],
[actor.cylinder, {'directions': dirs}]]
[actor.cylinder, {'directions': dirs}],
[actor.disk, {'directions': dirs}]]

scene = window.Scene()

for act_func, extra_args in actor_list:
colors = np.array([[1, 0, 0, 0.3], [0, 1, 0, 0.4], [1, 1, 0, 1]])
heights = np.array([5, 7, 10])

geom_actor = act_func(centers=xyz, heights=heights, colors=colors[:],
**extra_args)
if act_func == actor.disk:
geom_actor = act_func(centers=xyz, colors=colors[:], rinner=4,
router=8, rresolution=2, cresolution=10,
**extra_args)
else:
heights = np.array([5, 7, 10])
geom_actor = act_func(centers=xyz, heights=heights, colors=colors[:],
**extra_args)
scene.add(geom_actor)

if interactive:
Expand All @@ -945,8 +951,12 @@ def test_advanced_geometry_actor(interactive=False):
heights = 10

scene.clear()
geom_actor = act_func(centers=xyz[:, :3], heights=10, colors=colors[:],
**extra_args)
if act_func == actor.disk:
Sassafrass6 marked this conversation as resolved.
Show resolved Hide resolved
geom_actor = act_func(centers=xyz[:, :3], colors=colors[:],
**extra_args)
else:
geom_actor = act_func(centers=xyz[:, :3], heights=10, colors=colors[:],
**extra_args)
scene.add(geom_actor)

if interactive:
Expand Down
12 changes: 9 additions & 3 deletions fury/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
CellArray, PolyDataNormals, Actor, PolyDataMapper,
Matrix4x4, Matrix3x3, Glyph3D, VTK_DOUBLE, VTK_FLOAT,
Transform, AlgorithmOutput, VTK_INT, VTK_UNSIGNED_CHAR,
IdTypeArray)
TransformPolyDataFilter, IdTypeArray)


def remove_observer_from_actor(actor, id):
Expand Down Expand Up @@ -706,7 +706,7 @@ def get_actor_from_primitive(vertices, triangles, colors=None,


def repeat_sources(centers, colors, active_scalars=1., directions=None,
source=None, vertices=None, faces=None):
source=None, vertices=None, faces=None, orientation=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing the orientation argument is not possible without a test on repeat_sources.

Yes, please do add a test in fury.tests.test_utils.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your implementation of test_advanced_geometry_actor is nice, thank you. I've committed the change.

repeat_sources is used by several actors. Testing its functionality requires careful thought and definitely some effort.
Because I did not author this routine, and am only trying to submit a bug fix to the code, I respectfully decline.

Copy link
Contributor

@guaje guaje Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. You have addressed all my comments. IDK if @skoudoro has anything else to add.

"""Transform a vtksource to glyph."""
if source is None and faces is None:
raise IOError("A source or faces should be defined")
Expand Down Expand Up @@ -749,10 +749,16 @@ def repeat_sources(centers, colors, active_scalars=1., directions=None,

glyph = Glyph3D()
if faces is None:
if orientation is not None:
transform = Transform()
transform.SetMatrix(numpy_to_vtk_matrix(orientation))
rtrans = TransformPolyDataFilter()
rtrans.SetInputConnection(source.GetOutputPort())
rtrans.SetTransform(transform)
source = rtrans
glyph.SetSourceConnection(source.GetOutputPort())
else:
glyph.SetSourceData(polydata_geom)

glyph.SetInputData(polydata_centers)
glyph.SetOrient(True)
glyph.SetScaleModeToScaleByScalar()
Expand Down