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

Ellipsoid actor implemented with SDF #791

Merged
merged 11 commits into from Jul 19, 2023

Conversation

tvcastillod
Copy link
Contributor

Hi, this PR aims to create a new ellipsoid actor defined with SDF and raymarching. The goal is to make an implementation that allows displaying a large amount of data, with good visual quality, and without compromising performance. I'm still working on improvements and ways to test the implementation.

@guaje guaje requested review from a team and removed request for a team June 7, 2023 16:50
@skoudoro
Copy link
Contributor

skoudoro commented Jun 7, 2023

Hi @guaje,

I see that you requested a review from all but I see that this PR is a draft with WIP tag.

Is this PR ready for review? What is the status ? @tvcastillod, can you answer also ?

@tvcastillod
Copy link
Contributor Author

Is this PR ready for review? What is the status ? @tvcastillod, can you answer also ?

@skoudoro Not yet. I was waiting for today's meeting to know how to proceed with this PR, I received some comments that I will address soon, and when ready I will change the status and request the review again.

@skoudoro
Copy link
Contributor

skoudoro commented Jun 7, 2023

Thank you for the feedback!

@tvcastillod tvcastillod removed the request for review from a team June 12, 2023 17:37
@tvcastillod tvcastillod marked this pull request as ready for review June 12, 2023 18:18
@tvcastillod tvcastillod changed the title [WIP] Ellipsoid actor implemented with SDF Ellipsoid actor implemented with SDF Jun 12, 2023
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.

Hi @tvcastillod,

Here is an initial review. I will try a bit later your PR

fury/actors/ellipsoid.py Outdated Show resolved Hide resolved
fury/actors/ellipsoid.py Outdated Show resolved Hide resolved
fury/actors/ellipsoid.py Outdated Show resolved Hide resolved
fury/actors/ellipsoid.py Outdated Show resolved Hide resolved
fury/shaders/sdf_impl.frag Outdated Show resolved Hide resolved
fury/tests/test_actors.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Merging #791 (0a49b68) into master (73e0a1a) will decrease coverage by 0.09%.
The diff coverage is 78.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
- Coverage   84.41%   84.33%   -0.09%     
==========================================
  Files          43       44       +1     
  Lines       10166    10356     +190     
  Branches     1381     1410      +29     
==========================================
+ Hits         8582     8734     +152     
- Misses       1227     1252      +25     
- Partials      357      370      +13     
Impacted Files Coverage Δ
fury/actor.py 85.59% <41.93%> (-1.20%) ⬇️
fury/actors/tensor.py 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

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.

Great job @tvcastillod, Looks good to me.

I have few questions below to clarify or correct and then I think this PR will be ready to be merged.

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

@skoudoro @guaje would you mind taking a last look to see if there is anything else I need to adjust?

@skoudoro
Copy link
Contributor

@JoaoDell, Can you look and review the shader part.

@ganimtron-10, can you look and review the python part.

Both, please, let @tvcastillod knows if you have any questions or if you see something.

After that, we can go ahead and merge this PR.

Copy link
Contributor

@JoaoDell JoaoDell left a comment

Choose a reason for hiding this comment

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

Just finished reviewing mainly the shader part. The tests worked and sucessfully rendered the ellipsoids, so it is working. I just got some questions to better understand some choices, but everything seems fine 👍

fury/tests/test_actors.py Outdated Show resolved Hide resolved
fury/tests/test_actors.py Outdated Show resolved Hide resolved
fury/actors/tensor.py Show resolved Hide resolved
fury/actors/tensor.py Show resolved Hide resolved
fury/actors/tensor.py Show resolved Hide resolved
Copy link
Contributor

@ganimtron-10 ganimtron-10 left a comment

Choose a reason for hiding this comment

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

Hey @tvcastillod,
I had the few below comments. Please them out then it would be ready to go.

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/actor.py Outdated Show resolved Hide resolved
fury/actors/tensor.py Outdated Show resolved Hide resolved
fury/actors/tensor.py Outdated Show resolved Hide resolved
fury/actors/tensor.py Outdated Show resolved Hide resolved
fury/actors/tensor.py Show resolved Hide resolved
@tvcastillod
Copy link
Contributor Author

@skoudoro I did all the requested changes, I think now it is ready.

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.

Thank you for this work @tvcastillod.

Thank you for the review @JoaoDell, @ganimtron-10 and @guaje. It is good to have other pair of eye

merging

@skoudoro skoudoro merged commit 2bba1b9 into fury-gl:master Jul 19, 2023
21 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants