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

Adding Anisotropy and Clear coat to PBR material #523

Merged
merged 36 commits into from
Jan 28, 2022

Conversation

guaje
Copy link
Contributor

@guaje guaje commented Jan 14, 2022

This PR adds the latest material modeling capabilities supported by VTK (see all the references here). In particular, this PR focuses on adding functions to model anisotropic materials (see more here) and adding a clear coat on top of an actor (see more here).

Here is a screenshot of the included tutorial:

image

And here is a gif of the included interactive demo:

PBR_Demo

@pep8speaks
Copy link

pep8speaks commented Jan 14, 2022

Hello @guaje! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 161:13: E265 block comment should start with '# '
Line 171:13: E265 block comment should start with '# '
Line 172:13: E265 block comment should start with '# '
Line 173:13: E265 block comment should start with '# '
Line 174:13: E265 block comment should start with '# '
Line 175:13: E265 block comment should start with '# '
Line 176:13: E265 block comment should start with '# '
Line 177:13: E265 block comment should start with '# '
Line 178:13: E265 block comment should start with '# '

Comment last updated at 2022-01-28 19:08:48 UTC

@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #523 (1fdcb58) into master (4edfa34) will increase coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
+ Coverage   82.11%   82.40%   +0.29%     
==========================================
  Files          54       54              
  Lines       10628    10997     +369     
  Branches     1061     1080      +19     
==========================================
+ Hits         8727     9062     +335     
- Misses       1641     1670      +29     
- Partials      260      265       +5     
Impacted Files Coverage Δ
fury/fury/data/fetcher.py 72.41% <0.00%> (-4.69%) ⬇️
fury/fury/io.py 82.38% <0.00%> (-0.42%) ⬇️
fury/fury/lib.py 100.00% <0.00%> (ø)
fury/fury/data/__init__.py 100.00% <0.00%> (ø)
fury/fury/tests/test_io.py 98.50% <0.00%> (+0.23%) ⬆️
fury/fury/tests/test_utils.py 95.00% <0.00%> (+1.56%) ⬆️
fury/fury/window.py 62.38% <0.00%> (+1.59%) ⬆️
fury/fury/utils.py 91.49% <0.00%> (+2.33%) ⬆️
fury/fury/tests/test_window.py 76.37% <0.00%> (+9.40%) ⬆️
fury/fury/material.py 43.65% <0.00%> (+12.30%) ⬆️

@skoudoro
Copy link
Contributor

Hi @guaje,

This is release week for FURY. What is the status of this PR? Will you be able to finish it?

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 @guaje,

Thank you for this.

You should make bigger windows in your tutorial. when it starts, they are really small.

See below for some other comments.

fury/window.py Outdated Show resolved Hide resolved
fury/window.py Show resolved Hide resolved
docs/examples/viz_pbr_interactive.py Show resolved Hide resolved
docs/examples/viz_pbr_interactive.py Outdated Show resolved Hide resolved
docs/tutorials/03_shaders/viz_pbr_spheres.py Outdated Show resolved Hide resolved
from fury.utils import set_input


def load_cubemap_texture(fnames, interpolate_on=True, mipmap_on=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

this function can be useful for our actors. after merging, we should create an issue to remove duplicate

fury/material.py Outdated Show resolved Hide resolved
fury/tests/test_window.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
docs/examples/viz_pbr_interactive.py Outdated Show resolved Hide resolved
@guaje guaje marked this pull request as ready for review January 27, 2022 19:10
fury/window.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Garyfallidis Garyfallidis left a comment

Choose a reason for hiding this comment

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

@guaje I added a few more comments.

fury/window.py Outdated Show resolved Hide resolved
the rendering of the skybox.
"""

scene = window.Scene(skybox_tex=cubemap, render_skybox=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use one parameter. Can you explain why two parameters are necessary?

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 will add an issue to remove the skybox since that might require extra functionality to notify the actors they should not use IBL anymore.

The update skybox should be easier since the light information is only calculated when we render the scene. However, I think this should come in a future PR too.

Copy link
Contributor

@Garyfallidis Garyfallidis left a comment

Choose a reason for hiding this comment

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

Only a few small things left @guaje !

def change_slice_anisotropy_rotation(slider):
global pbr_params, sphere
pbr_params['anisotropy_rotation'] = slider.value
sphere.GetProperty().SetAnisotropyRotation(
Copy link
Contributor

Choose a reason for hiding this comment

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

@guaje For such functions we will need utility functions in material.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.

Changed. PTAL

docs/examples/viz_pbr_interactive.py Outdated Show resolved Hide resolved
docs/examples/viz_pbr_interactive.py Outdated Show resolved Hide resolved
@skoudoro
Copy link
Contributor

Nice work, Thank you a lot for pushing @guaje! merging

@skoudoro skoudoro merged commit b3aa4d7 into fury-gl:master Jan 28, 2022
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.

4 participants