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

Support Int8, Int16, and Int32 textures #372

Merged
merged 9 commits into from
Feb 9, 2021
Merged

Conversation

manzt
Copy link
Member

@manzt manzt commented Feb 6, 2021

Fixes #370 more robustly..

The primary change in this PR is that we don't cast any TypedArrays unless there is no WebGL2 support. In addition, all of the handling of webgl2 vs webgl1 is isolated to just before creating a texture. This changes the previous behavior where both ImageLayer and MultiscaleImageLayer checked for WebGL2, now it's done in once place: dataToTexture.

The main weirdness seems to be that the text is black on the example image. @ilan-gold, I'm not sure if you know why this is the case.

image

@manzt manzt requested a review from ilan-gold February 6, 2021 18:11
@manzt
Copy link
Member Author

manzt commented Feb 6, 2021

Update: FWIW, I think this actually is the right behavior. This is the same image in napari:

image

@ilan-gold
Copy link
Collaborator

ilan-gold commented Feb 7, 2021

@manzt It's good that we agree with Napari. If you look at the picker tool in Avivator over those points, they are negative one, which causes the shader to render black simply because the output there is either less than 0 for the auto-slider values (which are 1 for each channel) or 0 if you move the slider to the min value of the domain (-1). But in theory, the sliders should be able to go from -127 -> 127 for this image (and they would be able to in Vitessce because we have the "Full" option). So we could have an image like this:
Screen Shot 2021-02-06 at 9 24 55 PM
Note the "Blue" slider value which is -114

Copy link
Collaborator

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

Looks good. Let's add that min option to DTYPE_VALUES so this works nicely with Vitessce's "full" option. Thanks!

src/constants.ts Show resolved Hide resolved
src/layers/utils.js Show resolved Hide resolved
@manzt
Copy link
Member Author

manzt commented Feb 9, 2021

@ilan-gold I added a JSDoc @deprecated tag to DTYPE_VALUES. This will add a strikethrough to uses of DTYPE_VALUES, DTYPE_VALUES, and offer a message that references my comment. (It will also show a strikethrough when referenced within Viv's source, but this happens in one place and the warning is intended for end users).

Screen Shot 2021-02-09 at 9 37 51 AM

@ilan-gold
Copy link
Collaborator

Ok! Thanks!

@manzt manzt merged commit 1ebf0ba into master Feb 9, 2021
@manzt manzt deleted the manzt/signed-types-actual branch February 9, 2021 16:36
@oeway
Copy link
Contributor

oeway commented Feb 9, 2021

@manzt Will you update it in vizarr too?

@manzt
Copy link
Member Author

manzt commented Feb 10, 2021

@oeway, yes when I get the chance. Hopefully this week, now that we've made a release.

@will-moore
Copy link

Just noticed with https://github.com/hms-dbmi/vizarr/pull/136/files that DTYPE_VALUES is still being used in vizarr (and linked here).

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.

Support signed types
4 participants