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

Conversation

Sassafrass6
Copy link
Contributor

Added necessary alignment between glyph creation and actor mapping. If a vtkSource is not aligned with the X axis, provide 4x4 orientation matrix to repeat_sources to align.

This is necessary for the Cylinder object, which is aligned to the global Y axis in vtk. Without the correct orientation of the glyph, rotation into the z plane is not possible.

Also, the DiskSource is default aligned to the Z axis in vtk.
A similar routine for plotting discs (i.e. another use case for this patch) is included in actor.py.

Cylinder's with same specified direction as Arrows, original:
Screen Shot 2022-03-03 at 5 28 53 PM

Trivial implementation of Disks (Non-functional in a similar manner to cylinders):
Screen Shot 2022-03-03 at 5 35 00 PM

Correct implementation of Cylinders and Disks:
Screen Shot 2022-03-03 at 5 26 16 PM

… and actor mapping. If a vtkSource is not aligned with the X axis, provide 4x4 orientation matrix to repeat_sources to align
@pep8speaks
Copy link

pep8speaks commented Mar 4, 2022

Hello @Sassafrass6! 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-04-04 20:08:36 UTC

@skoudoro
Copy link
Contributor

skoudoro commented Mar 4, 2022

Hi @Sassafrass6,

Thank you for this PR. Quite a busy period for me, Not sure I will have time to look at your PR this week or next week.

@guaje, Can you look at this, please?

@skoudoro
Copy link
Contributor

skoudoro commented Mar 4, 2022

Could you also address all pep8 issues as you can see here

@codecov
Copy link

codecov bot commented Mar 4, 2022

Codecov Report

Merging #551 (c0c2874) into master (dadbb8e) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
- Coverage   88.61%   88.55%   -0.06%     
==========================================
  Files          55       55              
  Lines       10953    10979      +26     
  Branches     1080     1083       +3     
==========================================
+ Hits         9706     9723      +17     
- Misses        952      959       +7     
- Partials      295      297       +2     
Impacted Files Coverage Δ
fury/fury/data/fetcher.py 71.03% <0.00%> (-1.38%) ⬇️
fury/fury/io.py 81.46% <0.00%> (-0.93%) ⬇️
fury/fury/actor.py 88.00% <0.00%> (-0.32%) ⬇️
fury/fury/tests/test_actors.py 91.42% <0.00%> (-0.01%) ⬇️
fury/fury/lib.py 100.00% <0.00%> (ø)
fury/fury/tests/test_io.py 98.51% <0.00%> (+0.01%) ⬆️
fury/fury/window.py 83.21% <0.00%> (+0.03%) ⬆️
fury/fury/utils.py 91.62% <0.00%> (+0.13%) ⬆️

@Sassafrass6
Copy link
Contributor Author

Sassafrass6 commented Mar 4, 2022

I have addressed all of the PEP8 issues.

EDIT: The latest commit corrects the issue with normals.
ksnip_20220304-103104

FIXED-There is apparently still some issue with the cylinder normals. They are not shaded as one would expect.

ksnip_20220304-085034

Copy link
Contributor

@guaje guaje left a comment

Choose a reason for hiding this comment

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

Hi @Sassafrass6

Thanks for catching and fixing this. I have added some specific comments and I would like to ask you to add a new function to test the new orientation parameter. You can add it to fury.tests.test_utils.py.

fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/tests/test_actors.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@guaje guaje left a comment

Choose a reason for hiding this comment

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

@Sassafrass6 I have added some additional requests based on your latest changes/commits.

fury/actor.py Outdated Show resolved Hide resolved
fury/tests/test_actors.py Outdated Show resolved Hide resolved
@@ -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.

@Sassafrass6
Copy link
Contributor Author

Sassafrass6 commented Mar 22, 2022

Any update on the acceptance of this PR?

The modification is critical, for the cylinder actor to function.

@skoudoro
Copy link
Contributor

Hi @Sassafrass6,

Apologize for not giving news. The core maintainers (including me) are really focused on this workshop that we are organizing: https://dipy.org/workshops/dipy-workshop-2022

That's why we were not so active these past 2 - 3 weeks. The workshop finishes this Friday, so everything will get back to normal, next week (review, update, etc...).

I agree this is an important update which might imply a quick-release also.

@skoudoro
Copy link
Contributor

skoudoro commented Apr 4, 2022

This is necessary for the Cylinder object, which is aligned to the global Y axis in vtk

Should not it be applied to all actors? I am surprised that only the cylinders are oriented along the Y-axis. I suppose this is the case for all vtksource. Which vtksource is aligned along the X-axis?

I need to look deeper at this, when I see this tutorial, they seem to have all the same orientations.

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

some additional comments. Thank you again @Sassafrass6.

I am currently checking some vtk features and after your update, it should be good to merge

Comment on lines -1612 to -1614
src = CylinderSource() if faces is None else None

if src 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.

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.

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

RGB or RGBA (for opacity) R, G, B and A should be at the range [0, 1]
rinner : float
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

fury/actor.py Outdated
disk inner radius, default: 0.3
router : float
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

fury/actor.py Outdated
disk outer radius, default: 0.5
cresolution: int, default: 6
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

@Sassafrass6
Copy link
Contributor Author

Sassafrass6 commented Apr 4, 2022

This is necessary for the Cylinder object, which is aligned to the global Y axis in vtk

Should not it be applied to all actors? I am surprised that only the cylinders are oriented along the Y-axis. I suppose this is the case for all vtksource. Which vtksource is aligned along the X-axis?

I need to look deeper at this, when I see this tutorial, they seem to have all the same orientations.

In this example that you have linked, the arrow and cone are aligned along x. The cylinder is aligned along y, and the disk is aligned such that the normal is along z.
The camera is oriented the same way as FURY defaults to, with x to the right, y upward, and the z axis out of the monitor.
The other actors implemented within FURY are all correctly aligned on x, as far as I can tell.

I'm making the format changes now.

-Frank

Update: Changes committed

@skoudoro
Copy link
Contributor

skoudoro commented Apr 4, 2022

In this example that you have linked, the arrow and cone are aligned along x. The cylinder is aligned along y, and the disk is aligned such that the normal is along z.

I confirm.

This is ready to go as soon as you address my last comment.

@skoudoro skoudoro merged commit 8335555 into fury-gl:master Apr 4, 2022
@skoudoro
Copy link
Contributor

skoudoro commented Apr 4, 2022

Thank you for this useful update @Sassafrass6

@skoudoro skoudoro added the type:Bug Fix Something isn't working label Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Bug Fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants