Fix material docstrings, improved standard parameters and improved materials application support #488

guaje · 2021-08-10T14:53:34Z

This PR includes many improvements for the material module. Among many things it:

Addresses Mismatch in parameter and docstring in manifest_standard() in material module #469.
Add all the parameters supported by the different shading methods in VTK.
Improves test of existing materials.

codecov · 2021-08-11T00:18:37Z

Codecov Report

Merging #488 (43f7d94) into master (b2512b9) will increase coverage by 0.46%.
The diff coverage is 86.48%.

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   88.53%   89.00%   +0.46%     
==========================================
  Files          31       33       +2     
  Lines        6575     7092     +517     
  Branches      787      834      +47     
==========================================
+ Hits         5821     6312     +491     
- Misses        535      547      +12     
- Partials      219      233      +14

Impacted Files	Coverage Δ
fury/material.py	`83.33% <86.48%> (+8.33%)`	⬆️
fury/utils.py	`85.85% <0.00%> (-0.70%)`	⬇️
fury/layout.py	`100.00% <0.00%> (ø)`
fury/shaders/__init__.py	`100.00% <0.00%> (ø)`
fury/actors/peak.py	`90.00% <0.00%> (ø)`
fury/actors/tests/test_peak.py	`97.95% <0.00%> (ø)`
fury/shaders/base.py	`91.54% <0.00%> (+0.16%)`	⬆️
fury/primitive.py	`92.13% <0.00%> (+0.35%)`	⬆️
fury/actor.py	`89.63% <0.00%> (+0.59%)`	⬆️
fury/window.py	`81.95% <0.00%> (+1.79%)`	⬆️

skoudoro

Hi @guaje,

Thank you for this fix. See below for some comments. It is almost ready to go.

skoudoro · 2021-09-03T11:12:02Z

fury/material.py

+        warnings.warn('Your PBR effect can not be apply due to VTK version. '
+                      'Please upgrade your VTK version (should be >= 9.0.0).')


Why this change?

skoudoro · 2021-09-03T11:14:00Z

fury/material.py

@@ -60,22 +60,30 @@ def manifest_standard(actor, ambient_level=0, ambient_color=(1, 1, 1),
        fragment level.

    """
-    prop = actor.GetProperty()
+    try:


Why do you need a try? which actor does not have this feature?

This was added because contour_from_label was throwing an exception when trying to access the Property object. See test_material and test_manifest_standard.

ok, I will look into it

fury/material.py

skoudoro · 2021-09-03T11:16:24Z

fury/material.py

+            warnings.warn(
+                'Unknown interpolation. Ignoring "{}" interpolation option. '
+                'And using the default ("{}") option.'.format(
+                    interpolation, 'gouraud'), UserWarning)


not sure the warning will be written correctly this way. I need to test it

What do you mean? It's created according to this

As you can see below, the format does not work correctly (it does not work well with multiline) :

In [1]: from fury import actor, material In [2]: act = actor.axes() In [3]: material.manifest_standard(act, interpolation="test") /Users/koudoro/Software/fury/fury/material.py:78: UserWarning: Unknown interpolation. Ignoring "test" interpolation option. And using the default ("gouraud") option. interpolation, 'gouraud'), UserWarning)

I see, I'll look into it.

fury/tests/test_material.py

pep8speaks · 2021-09-06T20:48:30Z

Hello @guaje! 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 2021-09-07 21:56:56 UTC

Garyfallidis

@guaje I see that there is no tutorial in this PR. I suppose that this is on purpose?

fury/material.py

Garyfallidis · 2021-09-07T00:53:59Z

fury/tests/test_material.py

+        npt.assert_array_almost_equal(actual, desired, decimal=2)
+
+        material.manifest_standard(test_actor)
+        window.record(scene, out_path=tmp_fname, size=(200, 200),


Interesting that you prefer using record than snapshot. Snapshot returns directly the framebuffer as a numpy array.

Thanks, I didn't think of it. However, I found this problem (#467) when using window.snapshot().

Garyfallidis · 2021-09-07T01:06:07Z

@guaje Can you explain how I would have to change the following code to see the PBR effect?

from fury import actor, window, material


scene = window.Scene()

ax = actor.axes()

scene.add(ax)

material.manifest_pbr(ax, roughness=0.)

window.show(scene)

Garyfallidis · 2021-09-08T16:53:02Z

Thank you for this PR @guaje !
Please remember to update the tests after the snapshot function is resolved as reported here #467

Garyfallidis · 2021-09-08T16:53:59Z

Please also make another PR to improve manifest_pbr function to support actors that are created from primitives.

guaje added 4 commits August 2, 2021 09:43

Added corrections on docstring.

ff2b6fc

Added missing phong parameters.

121bdae

Improved documentation for standard material.

048bc83

Minor changes in PBR material.

8b5c0e2

guaje added 7 commits August 12, 2021 14:05

Removed irrelevant passing tests from standard material test.

8085610

Removed unused code on standard material test.

661414b

Finished standard material tests.

f364de7

Replaced nibabel TemporaryDirectory with python3's.

d05089b

Replaced equal asserts with almost_equal.

732c998

Replaced almost_equal asserts with array_almost_equal.

e05765c

Some test colors have been converted to the 0-1 range.

8d09616

skoudoro reviewed Sep 3, 2021

View reviewed changes

skoudoro added the type:Bug Fix Something isn't working label Sep 3, 2021

guaje added 3 commits September 3, 2021 16:36

Trying assert_array_almost_equal with 2 decimal digits flexibility.

2ac8f15

Updated standard tests to check for 2 decimals.

e6bb8f4

Added VTK<9 test for PBR material.

ba53713

Improved interpolation warning. Removed unnecessary PBR tests.

74fa167

Garyfallidis reviewed Sep 7, 2021

View reviewed changes

Added PBR tests. Fixed material warnings.

43f7d94

guaje changed the title ~~[WIP] Fix material issues~~ Fix material issues Sep 7, 2021

guaje marked this pull request as ready for review September 7, 2021 22:00

guaje changed the title ~~Fix material issues~~ Fix material docstrings and improving materials support Sep 8, 2021

guaje changed the title ~~Fix material docstrings and improving materials support~~ Fix material docstrings, improved standard parameters and improved materials application support Sep 8, 2021

Garyfallidis merged commit dcfbfa1 into fury-gl:master Sep 8, 2021

guaje deleted the fix_material_doc branch September 9, 2021 21:04

skoudoro linked an issue Sep 16, 2021 that may be closed by this pull request

Mismatch in parameter and docstring in manifest_standard() in material module #469

Closed

skoudoro mentioned this pull request Sep 16, 2021

Mismatch in parameter and docstring in manifest_standard() in material module #469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix material docstrings, improved standard parameters and improved materials application support #488

Fix material docstrings, improved standard parameters and improved materials application support #488

guaje commented Aug 10, 2021 •

edited

codecov bot commented Aug 11, 2021 •

edited

skoudoro left a comment

skoudoro Sep 3, 2021

skoudoro Sep 3, 2021

guaje Sep 3, 2021

skoudoro Sep 3, 2021

skoudoro Sep 3, 2021

guaje Sep 3, 2021

skoudoro Sep 3, 2021

guaje Sep 3, 2021

pep8speaks commented Sep 6, 2021 •

edited

Garyfallidis left a comment

Garyfallidis Sep 7, 2021

guaje Sep 7, 2021

Garyfallidis commented Sep 7, 2021

Garyfallidis commented Sep 8, 2021

Garyfallidis commented Sep 8, 2021 •

edited

		warnings.warn('Your PBR effect can not be apply due to VTK version. '
		'Please upgrade your VTK version (should be >= 9.0.0).')

Fix material docstrings, improved standard parameters and improved materials application support #488

Fix material docstrings, improved standard parameters and improved materials application support #488

Conversation

guaje commented Aug 10, 2021 • edited

codecov bot commented Aug 11, 2021 • edited

Codecov Report

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Sep 6, 2021 • edited

Comment last updated at 2021-09-07 21:56:56 UTC

Garyfallidis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garyfallidis commented Sep 7, 2021

Garyfallidis commented Sep 8, 2021

Garyfallidis commented Sep 8, 2021 • edited

guaje commented Aug 10, 2021 •

edited

codecov bot commented Aug 11, 2021 •

edited

pep8speaks commented Sep 6, 2021 •

edited

Garyfallidis commented Sep 8, 2021 •

edited