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

Change antialiasing default to ssaa #4521

Merged
merged 10 commits into from
Jun 28, 2023
Merged

Conversation

banesullivan
Copy link
Member

@banesullivan banesullivan commented Jun 10, 2023

I use a custom build of VTK for performant off screen rendering with my GPU - an "EGL" build. A handful of PyVista's defaults and tests have been incompatible with this build. These changes rectify that by changing the default anti-aliasing to ssaa instead of fxaa -- @akaszynski, I'd appreciate your thoughts on this change and if its okay?

Resolves #4524

@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Jun 10, 2023
@banesullivan banesullivan changed the title Improve VTK EGL build compatibility Change antialiasing default to ssaa Jun 10, 2023
@banesullivan banesullivan marked this pull request as draft June 10, 2023 21:18
@akaszynski
Copy link
Member

These changes rectify that by changing the default anti-aliasing to ssaa instead of fxaa -- @akaszynski, I'd appreciate your thoughts on this change and if its okay?

This will improve base plotting and I think that for most cases and configurations, this will work.

I have a few weaker computers laying around and I'd like to test it out to make sure that our base configuration works on 10 year old integrated GPUs. If that's the case, there's zero reason to keeping this around.

There's a second reason for this as well: it makes our plots look much better. MSAA support seems inconsistent between GPUs, and FXAA doesn't look as good as SSAA.

Here's a hot take since we're talking about themes: why not enable anti-aliasing by default? This is more controversial than just changing the default, but I really feel that having plots look awesome out of the box should be the default. We can then have a theme that's more suited for low power GPUs.

@banesullivan
Copy link
Member Author

I really feel that having plots look awesome out of the box should be the default. We can then have a theme that's more suited for low power GPUs

Completely agree here -- let's go for this

@banesullivan banesullivan marked this pull request as ready for review June 13, 2023 16:48
@banesullivan
Copy link
Member Author

NOTE: to validate this is not breaking the docs, we should trigger a "workflow dispatch" job for the documentation build disabling the cache.

@banesullivan
Copy link
Member Author

So maybe enabling antialiasing breaks things like picking... might need to rethink enabling it by default

@banesullivan banesullivan marked this pull request as draft June 13, 2023 20:18
@akaszynski
Copy link
Member

So maybe enabling antialiasing breaks things like picking... might need to rethink enabling it by default

This means there's an issue with how we assemble our render passes, which requires disabling the existing passes.

@banesullivan
Copy link
Member Author

Ah this has been tracked in #2520

@akaszynski
Copy link
Member

akaszynski commented Jun 25, 2023

Ah this has been tracked in #2520

Sorta. There are indeed issues when enabling SSAA, but it's not in #2520. One I've noticed off-hand is pressing "f" no longer focuses on the mesh. However, enabling picking (for example enable_mesh_picking) causes pressing "f" to both focus on the picked point and to trigger picking it. There's a bit to debug there, and I'd like to have this resolved in a separate PR.

Note: Pressing "f" without enabling SSAA still has the effect of picking and focusing on the mesh.

@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Merging #4521 (2a1731b) into main (518f322) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4521      +/-   ##
==========================================
- Coverage   95.77%   95.77%   -0.01%     
==========================================
  Files         127      127              
  Lines       21357    21349       -8     
==========================================
- Hits        20454    20446       -8     
  Misses        903      903              

@banesullivan banesullivan marked this pull request as ready for review June 25, 2023 21:43
@banesullivan
Copy link
Member Author

Looks like tests are now passing after #4254

@akaszynski
Copy link
Member

Interesting... After #4254, pressing "f" no longer focuses on the point under the cursor (regardless of the settings of anti-aliasing).

@akaszynski
Copy link
Member

Interesting... After #4254, pressing "f" no longer focuses on the point under the cursor (regardless of the settings of anti-aliasing).

Got it! Simply setting the default picker to the interactor fixes this regression:

import pyvista as pv
from pyvista.plotting.opts import PickerType

sphere = pv.Sphere(center=(1, 0, 0))

pl = pv.Plotter()
pl.add_mesh(sphere, color='lightblue', show_edges=True)
# pl.enable_anti_aliasing('ssaa')  # works with and without anti-aliasing
pl.iren.picker = PickerType.POINT
pl.show()

@banesullivan banesullivan merged commit f8f0ae5 into main Jun 28, 2023
24 checks passed
@banesullivan banesullivan deleted the maint/egl-anti-aliasing branch June 28, 2023 02:07
@banesullivan banesullivan mentioned this pull request Jun 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_plot_cell has a very high variance
2 participants