Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add shading of Poly3DCollection#23914

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
timhoffm merged 1 commit intomatplotlib:mainfromoscargus:shaderefactor
Nov 1, 2022

Conversation

oscargus
Copy link
Member

@oscargusoscargus commentedSep 16, 2022
edited
Loading

PR Summary

Fixes#19509.

Refactors shading of Poly3DCollection and moves it to the constructor of the collection rather than in every individual plotting method, Can still be done anywhere though.

PR Checklist

Tests and Styling

  • [N/A] Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

@oscargusoscargus changed the titleRefactor shadingAdd shading of Poly3DCollectionSep 16, 2022
@oscargus
Copy link
MemberAuthor

Well, it is at least possible to import STL-files and add them to aPoly3DCollection with shading, but some things can be improved, like that it is required to provide a color the way the code is now.

image

@oscargusoscargus marked this pull request as ready for reviewOctober 22, 2022 10:51
@oscargusoscargus added this to thev3.7.0 milestoneOct 22, 2022
-------------------------------------

It is now possible to shade a `.Poly3DCollection`. This is useful if the
polygons are obtained from e.g. a 3D model.
Copy link
Member

Choose a reason for hiding this comment

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

Slightly unclear cause it's missing the general thing a 3D model is an example of.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I agree that this formulation is maybe not ideal. I was originally going to discuss things like STL-files, but since that is not the only way to represent a 3D model I kept it simple.

An option is to generate a static example loading an STL model (as we need additional libraries).

Copy link
Member

Choose a reason for hiding this comment

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

In what cases besides a 3D model would shading be useful?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I guess that one can have some other type of 3D polygons that are not necessarily considered to be a "3D model", although in some sense all 3D polygons could be considered a "3D model". One example may be voxels? Basically something that is not representing a physical object, but a visualization of something that happens to be 3D Polygons.

But I am open for suggestions.

@oscargusoscargusforce-pushed theshaderefactor branch 2 times, most recently from0c248a2 to7380621CompareOctober 23, 2022 11:46
@oscargus
Copy link
MemberAuthor

I had the idea of allowing bothedgecolor andedgecolors (similar forfacecolor/facecolors), but that breaks one of the tests resulting in the following image
voxels-edge-style

instead of

voxels-edge-style-expected

I cannot really judge if the expected behavior is really the bottom one and/or if it is a feature that passing the singular form does not apply shading?

@oscargus
Copy link
MemberAuthor

Shading of edgecolor is a bit dodgy by the way since it is not that straightforward to determine the normal of a line... (Not sure how it is actually done.)

Copy link
Member

@story645story645 left a comment

Choose a reason for hiding this comment

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

I'm nitpicking language you didn't write in the first place, so I can totally just open my own pr for that. I think the refactor into one place is great.

if facecolors is None and edgecolors in None:
_api.warn_external(
"You must provide facecolors, edgecolors, or both for "
"shade to work as expected.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"shade to work as expected.")
"shade to work.")

Should we error out instead of warn? It seems to be incorrect usage, and I don’t see a reason why a user might want to set shade without having the results shaded.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

The idea was that one can, sort of, expect the default colors to be used. However, those are currently not assigned until after the shading is applied. So I thought that a warning made more sense since this is something that can be changed in the future.

Copy link
Member

Choose a reason for hiding this comment

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

My point is

  • If people useshade=True, they want shade. I seeshade=True without giving colors as a plain usage error. IMHO it's better to fail loudly in that case rather than only warning and doing something else. - Warnings are often overlooked.
  • Erroring out is the more narrow API. We can always do something else later without breaking backward-compatibility. OTOH when we fall back to non-shading. Doing something else later is a backward-incompatible change.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Modulo a minor docstring suggestion.

Comment on lines 827 to 831
.. versionadded:: 3.7

.. note::
*facecolors* and/or *edgecolors* must be provided for shading
to work.
Copy link
Member

Choose a reason for hiding this comment

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

I think a note is more heavy than needed. We usually only state deprendencies between parameters in plain text:

Suggested change
..versionadded::3.7
..note::
*facecolors*and/or*edgecolors*mustbeprovidedforshading
towork.
Whenactivating*shade*,*facecolors*and/or*edgecolors*mustbe
provided.
..versionadded::3.7

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sure. Again this comes from the fact that ideally it should work without providing it, so I thought it was worth highlighting. But as it now errors as well, it is hard to miss it.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@story645story645story645 approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.

Adding lightsource when plotting Poly3DCollection
3 participants
@oscargus@story645@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp