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

Allow both 'auto' and None as texture_format when choosing VisPy node #6652

Merged
merged 4 commits into from Feb 13, 2024

Conversation

Czaki
Copy link
Collaborator

@Czaki Czaki commented Feb 8, 2024

Description

In #6411, we missed these lines:

if (
texture_format == 'auto'
and 'texture_float' not in get_gl_extensions()
):
# if the GPU doesn't support float textures, texture_format auto
# WILL fail on float dtypes
# https://github.com/napari/napari/issues/3988
texture_format = None

These were added in #3990 because 'auto' has a slightly different meaning in VisPy than really fully auto: it chooses the default texture dtype for the input NumPy array's dtype. For float arrays, this is float. However, if your OpenGL implementation doesn't support float textures, VisPy will raise.

Instead, passing texture_format=None when creating the ImageNode tells VisPy to transform the data to whatever texture format it sees fit.

In #6411, when getting the VisPy node for a given dtype, we only check for texture_format != 'auto' as the "catch-all" texture format. But, in fact, if we are on a machine that doesn't support float32 textures, by this point in the code the format has been changed to None, incorrectly triggering these lines:

if (
res.texture_format != "auto"
and dtype is not None
and _VISPY_FORMAT_TO_DTYPE[res.texture_format] != dtype
):
# it is a bug to hit this error — it is here to catch bugs
# early when we are creating the wrong nodes or
# textures for our data
raise ValueError(
trans._(
"dtype {dtype} does not match texture_format={texture_format}",
dtype=dtype,
texture_format=res.texture_format,
)
)

and causing a ValueError.

This PR fixes that by also checking for None in that same clause.

This PR also adds a test by monkeypatching the function that checks for float32 texture support.

References

#3988
#3990

@Czaki Czaki added the bugfix PR with bugfix label Feb 8, 2024
@Czaki Czaki added this to the 0.5.0 milestone Feb 8, 2024
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4f4c063) 92.32% compared to head (d3ed995) 92.27%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6652      +/-   ##
==========================================
- Coverage   92.32%   92.27%   -0.05%     
==========================================
  Files         605      605              
  Lines       54227    54231       +4     
==========================================
- Hits        50064    50044      -20     
- Misses       4163     4187      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@psobolewskiPhD
Copy link
Member

@Czaki
Copy link
Collaborator Author

Czaki commented Feb 8, 2024

Not possibly, but it is exactly this error.

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/opening-image-in-napari-linux/91950/3

@leopold-franz
Copy link
Contributor

Thank you so much for adding a fix! I will try it now.

@leopold-franz
Copy link
Contributor

leopold-franz commented Feb 8, 2024

I just tried running it (unfortunately i do not have direkt desktop access so I had to use x11 forwarding & xquartz to test this) and im running into this error now:

(follicle-tracker) labadmin@tph31-52-59:~$ napari                 
libGL error: No matching fbConfigs or visuals found              
libGL error: failed to load driver: swrast                 
WARNING: QOpenGLWidget: Failed to create context             
WARNING: composeAndFlush: QOpenGLContext creation failed                                                                                                                                
18:09:28 : WARNING : MainThread : composeAndFlush: QOpenGLContext creation faile                                                                                                                                                                                       
WARNING: composeAndFlush: makeCurrent() failed                                                                                                                                          
18:09:28 : WARNING : MainThread : composeAndFlush: makeCurrent() failed                                                                                                                 
WARNING: composeAndFlush: makeCurrent() failed                          
18:09:28 : WARNING : MainThread : composeAndFlush: makeCurrent() failed

@psobolewskiPhD
Copy link
Member

That looks like a driver issue ☹️

Also, have you gotten napari to work via X11 and xquartz? I always get OpenGL issue and blank screen.
I've had better luck with Xpra, as our docker container uses, and web client.

@Czaki
Copy link
Collaborator Author

Czaki commented Feb 8, 2024

OpenGL does not work well with x11 screen forwarding. To use napari remote, I use nomachine/anydesk.

@github-actions github-actions bot added the tests Something related to our tests label Feb 9, 2024
@jni jni changed the title Fix check of texture format. Allow both 'auto' and None as texture_format when choosing VisPy node Feb 12, 2024
napari/_vispy/_tests/test_vispy_image_layer.py Outdated Show resolved Hide resolved
@jni jni added the ready to merge Last chance for comments! Will be merged in ~24h label Feb 12, 2024
Czaki and others added 2 commits February 12, 2024 14:48
@jni jni merged commit ab226b9 into napari:main Feb 13, 2024
31 of 32 checks passed
@jni jni deleted the fix_checking_texture_type branch February 13, 2024 08:57
@jni jni removed the ready to merge Last chance for comments! Will be merged in ~24h label Feb 13, 2024
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/error-running-napari-on-hpc-could-not-load-qt-platform-plugins/92012/10

@Czaki Czaki modified the milestones: 0.5.0, 0.4.20 Feb 20, 2024
Czaki added a commit that referenced this pull request Feb 20, 2024
…#6652)

# Description

In #6411, we missed these lines:

https://github.com/napari/napari/blob/4f4c063ae5dd79d6d188e201d44b8d57eba71909/napari/_vispy/layers/image.py#L25-L32

These were added in #3990 because 'auto' has a slightly different
meaning in VisPy than *really* fully auto: it chooses the default
texture dtype for the input NumPy array's dtype. For float arrays, this
is float. However, if your OpenGL implementation doesn't support float
textures, VisPy will raise.

Instead, passing `texture_format=None` when creating the ImageNode tells
VisPy to transform the data to whatever texture format it sees fit.

In #6411, when getting the VisPy node for a given dtype, we only check
for `texture_format != 'auto'` as the "catch-all" texture format. But,
in fact, if we are on a machine that doesn't support float32 textures,
by this point in the code the format has been changed to None,
incorrectly triggering these lines:

https://github.com/napari/napari/blob/89f8194d3fa4eef620755804806ac69ef684df63/napari/_vispy/layers/image.py#L59-L73

and causing a ValueError.

This PR fixes that by also checking for None in that same clause.

This PR also adds a test by monkeypatching the function that checks for
float32 texture support.

# References

#3988
#3990

---------

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants