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

Fix active normals for properties point_normals, cell_normals, and method plot_normals with smooth_shading #6062

Merged
merged 10 commits into from May 13, 2024

Conversation

user27182
Copy link
Contributor

Resolves #6061
Resolves #2921
Resolves #6058

tests/plotting/test_plotting.py Outdated Show resolved Hide resolved
tests/plotting/test_plotting.py Outdated Show resolved Hide resolved
@pyvista-bot pyvista-bot added the bug Uh-oh! Something isn't working as expected. label May 9, 2024
Co-authored-by: Tetsuo Koyama <tkoyama010@gmail.com>
Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.99%. Comparing base (c4359ae) to head (c61f427).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6062      +/-   ##
==========================================
+ Coverage   96.98%   96.99%   +0.01%     
==========================================
  Files         141      141              
  Lines       24639    24640       +1     
==========================================
+ Hits        23895    23899       +4     
+ Misses        744      741       -3     

@user27182
Copy link
Contributor Author

Note: the test_backface_params test uses pv.ParametricCatalanMinimal(), which by default generates its own Normals. This appears to be common with the geometric source objects (even pv.Sphere() has a Normals array by default).

So, that test was failing because with this PR, the "stock" normals are now used, resulting in a different rendering. The new rendered image is updated in 2bfc4a1

pyvista/core/pointset.py Outdated Show resolved Hide resolved
Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

LGTM. This is largely a bug, but it also includes an element of breaking change, so it will be labeled and made known in the release notes.

@tkoyama010 tkoyama010 enabled auto-merge (squash) May 12, 2024 23:13
@tkoyama010 tkoyama010 added the breaking-change Changes that break backwards compatibility, especially changed default parameters. label May 12, 2024
@tkoyama010 tkoyama010 merged commit 0b44c29 into pyvista:main May 13, 2024
27 checks passed
@user27182 user27182 deleted the fix/active_normals branch May 13, 2024 02:29
tkoyama010 added a commit that referenced this pull request May 14, 2024
…d method `plot_normals` with `smooth_shading` (#6062)

* Fix point_normals and cell_normals properties

* Add plot_normals test

* Add fix for smooth shading

* Update test

* Update docs

* Apply suggestions from code review

Co-authored-by: Tetsuo Koyama <tkoyama010@gmail.com>

* Update backface param test image

---------

Co-authored-by: Tetsuo Koyama <tkoyama010@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes that break backwards compatibility, especially changed default parameters. bug Uh-oh! Something isn't working as expected.
Projects
None yet
3 participants