Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

What does this implement/fix?

This PR enables smooth shading for the pyvista 3d backend by default. This feature can be disabled by passing smooth_shading=False at the creation of the _Renderer.

Additional information

With smooth shading:

image

Without smooth shading:

image

@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #6435 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6435      +/-   ##
==========================================
+ Coverage   89.27%   89.27%   +<.01%     
==========================================
  Files         411      411              
  Lines       74517    74518       +1     
  Branches    12317    12317              
==========================================
+ Hits        66525    66526       +1     
  Misses       5135     5135              
  Partials     2857     2857

@GuillaumeFavelier GuillaumeFavelier changed the title Enable smooth shading for pyvista backend MRG: Enable smooth shading for pyvista backend Jun 7, 2019
@GuillaumeFavelier
Copy link
Contributor Author

What do you think @larsoner , @agramfort ?


def __init__(self, fig=None, size=(600, 600), bgcolor=(0., 0., 0.),
name=None, show=False):
name=None, show=False, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

I don't much like kwargs because it's easy to "lose" arguments. Why not just smooth_shading=True here?

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 wanted to use this property of kwargs to introduce backend specific parameter actually, you don't like the idea?
Of course, I can just put it to a plain parameter indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is your opinion on this @agramfort ?

| 3D function: | mayavi | pyvista |
+======================================+========+=========+
| :func:`plot_source_estimates` | ✓ | |
| :func:`plot_source_estimates` | ✓ | - |
Copy link
Member

Choose a reason for hiding this comment

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

it's partially implemented already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! It's not on this PR but in here with the Brain object, I'll remove it.

+--------------------------------------+--------+---------+
| :func:`snapshot_brain_montage` | ✓ | ✓ |
+--------------------------------------+--------+---------+
| :func:`plot_evoked_field` | ✓ | |
Copy link
Member

Choose a reason for hiding this comment

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

What happened to plot_evoked_field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appeared twice on the table for some reason, I just correct it in here.

@agramfort
Copy link
Member

why not making smooth shading the default unless it's not supported? then no need for a parameter :)

@agramfort agramfort merged commit 1f25533 into mne-tools:master Jun 8, 2019
@agramfort
Copy link
Member

thx @GuillaumeFavelier !

@GuillaumeFavelier GuillaumeFavelier deleted the pyvista_smooth_shading branch July 2, 2019 13:38
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.

3 participants