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

WIP: Port 3d testing to BackgroundPlotter completely #7697

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Apr 29, 2020

This PR ports testing of the PyVista 3d backend to the BackgroundPlotter completely.

As a consequence, the variable MNE_3D_BACKEND_TESTING needs refactoring.

Known issue

  • The dimensions of a screenshot taken with the BackgroundPlotter are not equal to win_height x win_width x 3 but slightly less. I suppose that the real dimensions taken into account are effectively those of the render view (which is smaller than the window).

@GuillaumeFavelier
Copy link
Contributor Author

I reported this behaviour in https://github.com/pyvista/pyvista/issues/703

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented May 4, 2020

One way to mitigate this issue is to use setMinimumSize() on the render view widget so that the window does not automatically shrink it.

@GuillaumeFavelier
Copy link
Contributor Author

the window does not automatically shrink it.

The drawback is that the user is no longer able to resize the window to a geometry smaller then window_size.

@GuillaumeFavelier
Copy link
Contributor Author

This can be relevant to #7211 (about minimum/fixed widget geometry)

@larsoner
Copy link
Member

larsoner commented May 4, 2020

The drawback is that the user is no longer able to resize the window to a geometry smaller then window_size.

Can you set this until the super init finishes, then un-set it?

@GuillaumeFavelier
Copy link
Contributor Author

I use it after BackgroundPlotter init to resize the render view. I'll see if there is a way to un-set it without updating the geometry of the window.

@larsoner
Copy link
Member

larsoner commented May 4, 2020

I use it after BackgroundPlotter init to resize the render view.

Can you see if it's possible to do before initializing the view? It should help prevent more jerky resizes, if those happen with the current approach

@GuillaumeFavelier
Copy link
Contributor Author

The window is resized before being shown so I don't think this will lead to jerkiness. But this is a workaround without a way to setting it at init. So far, locally the main issue I notice is with show_traces=True. When the window opens:

image

I have to manually resize to get all the content:

image

Also, the CIs do not seem to like those changes.

@larsoner
Copy link
Member

larsoner commented May 4, 2020

Probably whatever setMinSize you do for the 3D area, you need to do for the 2D area as well. Maybe at 1/4 of whatever the requested 3D vertical size is?

@larsoner
Copy link
Member

larsoner commented May 6, 2020

For now you could assert_allclose(shape, expected_shape, atol=40) # XXX undo once size fixed or so to get things to pass. Seems worth it to get the actual BackgroundPlotter used, then we can deal with fixing the size separately

@GuillaumeFavelier
Copy link
Contributor Author

Awesome idea, thank you. I was stuck with SetMinimumSize() tbh

@larsoner
Copy link
Member

larsoner commented May 6, 2020

The Azure failure is fixed by #7740, but I couldn't see the failed OSX log so I restarted that build.

@larsoner larsoner closed this May 6, 2020
@larsoner larsoner reopened this May 6, 2020
@larsoner
Copy link
Member

larsoner commented May 6, 2020

Failures on OSX apparently seem real

https://travis-ci.org/github/mne-tools/mne-python/builds/683971748

@GuillaumeFavelier
Copy link
Contributor Author

Actually if #7758 is chosen, I would prefer to keep testing on both plotters.

@GuillaumeFavelier GuillaumeFavelier deleted the renderer_full_background_plotter branch November 15, 2021 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants