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

ODF actor implemented with Ray Tracing #869

Merged
merged 147 commits into from
Nov 8, 2024
Merged

Conversation

tvcastillod
Copy link
Contributor

@tvcastillod tvcastillod commented Mar 8, 2024

This PR aims to create a new odf actor defined with raymarching based on the paper Ray Tracing Spherical Harmonics Glyphs . Work is still in progress.

A visual comparison with the current implementation of fury using polygons
image

An example is provided showing that it supports displaying multiple glyphs, and with different SH degree.
image

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.

Hello @tvcastillod, here are some minor comments on some of these files.

fury/actors/odf.py Outdated Show resolved Hide resolved
fury/actors/odf.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.

Hello @tvcastillod!

Overall great work putting up together such a needed actor.

In addition to the changes outlined below, I suggest moving the content of rt_odfs to ray_tracing/odf.

fury/texture/tests/test_utils.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
fury/shaders/utils/find_roots.frag Outdated Show resolved Hide resolved
fury/shaders/utils/find_roots.frag Outdated Show resolved Hide resolved
fury/shaders/utils/find_roots.frag Outdated Show resolved Hide resolved
fury/shaders/rt_odfs/tournier/eval_sh_12.frag Outdated Show resolved Hide resolved
fury/shaders/rt_odfs/tournier/eval_sh_10.frag Outdated Show resolved Hide resolved
fury/shaders/rt_odfs/tonemap.frag Outdated Show resolved Hide resolved
fury/actors/odf.py Outdated Show resolved Hide resolved
fury/shaders/rt_odfs/eval_sh.frag Outdated Show resolved Hide resolved
@tvcastillod tvcastillod marked this pull request as ready for review September 20, 2024 16:27
@guaje guaje requested a review from skoudoro September 23, 2024 22:28
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.

exciting, thank you for this work @tvcastillod !

First, can you fix the issues from code format CI's and build doc CI's

then, see my first initial comments below.

Question: Can you explain a bit how did you get all this eval_sh_*?

fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
@tvcastillod
Copy link
Contributor Author

Question: Can you explain a bit how did you get all this eval_sh_*?

Hi @skoudoro,

The eval_sh_* are taken directly from the source code given in the paper I took as reference, you can check their shadertoy demo for a quick look. These values correspond to the SH basis functions listed in Appendix A from the paper, which comes from the definition of Spherical Harmonics, the same from the DIPY documentation. What I added was the Tournier basis which is very similar to Descoteaux with some sign changes.

@skoudoro
Copy link
Contributor

Hi @tvcastillod,

Overall, it is almost ready to be merged. Can you fix Codespell CI's and Docs CI's which are failing...

In the docs CI, you have this 2 warnings:

/home/runner/work/fury/fury/fury/actor.py:docstring of fury.actor.odf:16: WARNING: Inline literal start-string without end-string.
/home/runner/work/fury/fury/fury/actors/odf.py:docstring of fury.actors.odf.sh_odf:14: WARNING: Inline literal start-string without end-string.

Concerning the codespell, you need to add shs as an ignore word to avoid the issue.

Thank you for the future update

@maharshi-gor
Copy link
Contributor

Hello @tvcastillod @guaje @skoudoro

Thanks for the work!

I tested this PR with a large dataset and when I tried to load the whole slice of that dataset. It worked perfectly fine. But if I zoom in it becomes distorted.
unnamed

NOTE

  1. The current version of fury ODF Slicer Actor was smooth and better. I am only adding one slice of data to the proposed odf actor as well.
  2. The current version of fury ODF Slicer Actor does not distort the image as well.

My Observation and Take away

  • I think it is something related to zoom and the shapes are not getting recalculated once zoomed in more.
  • We are doing something not right in the calculation implementation as it should be faster than the approach we have in fury.

Let me know if you need any information.

@guaje
Copy link
Contributor

guaje commented Oct 30, 2024

Hello @tvcastillod @guaje @skoudoro

Thanks for the work!

I tested this PR with a large dataset and when I tried to load the whole slice of that dataset. It worked perfectly fine. But if I zoom in it becomes distorted.
unnamed

NOTE

  1. The current version of fury ODF Slicer Actor was smooth and better. I am only adding one slice of data to the proposed odf actor as well.
  2. The current version of fury ODF Slicer Actor does not distort the image as well.

My Observation and Take away

  • I think it is something related to zoom and the shapes are not getting recalculated once zoomed in more.
  • We are doing something not right in the calculation implementation as it should be faster than the approach we have in fury.

Let me know if you need any information.

Thanks for this thorough review @maharshi-gor. Could you share the dataset and detailed steps to generate the error?

@maharshi-gor
Copy link
Contributor

Hello @guaje @tvcastillod,

It is a dataset called setsompop. Please find the Dataset here

Please fit the entire slice using CsaOdfModel and then zoom it.

Also, I would love to see an example from your-side to use the new odf actor with a odf model as the inputs are different from the current odf slicer actor. This might be an input issue!

Thanks.

@maharshi-gor
Copy link
Contributor

Hey @skoudoro

Let's merge this PR as for all the normal dataset it works fine. But for the large dataset @guaje and @tvcastillod are working on it.

I have created another issue to track that here.

@skoudoro
Copy link
Contributor

skoudoro commented Nov 8, 2024

Thank @maharshi-gor for the feedback, thanks @tvcastillod for this work and thanks @guaje for the review. merging

@skoudoro skoudoro merged commit 1d5e7dc into fury-gl:master Nov 8, 2024
13 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants