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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 15 commits into from
Apr 4, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
902d50a
Fixed #550 : Added necessary allignment between glyph creation and ac…
Sassafrass6 Mar 4, 2022
f9d2785
Changed typo in disk description, removed faces argument
Sassafrass6 Mar 4, 2022
5df2ff5
Adhere to PEP8 styling.
Sassafrass6 Mar 4, 2022
7a19085
Transform all PolyData, so that normals are preserved on surface of g…
Sassafrass6 Mar 4, 2022
077a683
Added tests for advanced geometry actors
Sassafrass6 Mar 7, 2022
da84d0f
Corrected typo in actor tests
Sassafrass6 Mar 7, 2022
67f0ee2
Corrected typo in actor tests
Sassafrass6 Mar 7, 2022
a089da0
Corrected typo in actor tests
Sassafrass6 Mar 7, 2022
5d256ff
Disks have no thickness, so directions must not be parallel or perpen…
Sassafrass6 Mar 7, 2022
d3bacd9
Advanced geometry test failed because disk direction was perpendicula…
Sassafrass6 Mar 8, 2022
7467437
Updated to Gauje's styling suggestions
Sassafrass6 Mar 9, 2022
8ddb19a
Added optional actor-specific arguments to actor tests
Sassafrass6 Mar 9, 2022
f18f9cc
Cleaned formatting in test_advanced_geometry, thanks to Gaujes sugges…
Sassafrass6 Mar 9, 2022
214103e
Forwarded faces
Sassafrass6 Mar 9, 2022
c0c2874
Disk description format update
Sassafrass6 Apr 4, 2022
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
View file
Open in desktop
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 •
edited
Loading

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, optional
Number of facets used to define perimeter of disk, default: 6
rresolution: int, optional
Number of facets used radially, default: 2
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=faces,
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
View file
Open in desktop
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
23 changes: 12 additions & 11 deletions fury/tests/test_actors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,20 +919,22 @@ 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}]]
heights = np.array([5, 7, 10])

actor_list = [[actor.cone, {'heights': heights, 'resolution': 8}],
[actor.arrow, {'heights': heights, 'resolution': 9}],
[actor.cylinder, {'heights': heights, 'resolution': 10}],
[actor.disk, {'rinner': 4, 'router': 8, 'rresolution': 2,
'cresolution': 2}]]

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)
geom_actor = act_func(xyz, dirs, colors, **extra_args)
scene.add(geom_actor)

if interactive:
Expand All @@ -941,12 +943,11 @@ def test_advanced_geometry_actor(interactive=False):
report = window.analyze_snapshot(arr, colors=colors)
npt.assert_equal(report.objects, 3)

scene.clear()

colors = np.array([1.0, 1.0, 1.0, 1.0])
heights = 10

scene.clear()
geom_actor = act_func(centers=xyz[:, :3], heights=10, colors=colors[:],
**extra_args)
geom_actor = act_func(xyz, dirs, colors, **extra_args)
scene.add(geom_actor)

if interactive:
Expand Down
12 changes: 9 additions & 3 deletions fury/utils.py
View file
Open in desktop
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 •
edited
Loading

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