-
-
Notifications
You must be signed in to change notification settings - Fork 378
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
feat(Rendering): Add support for halfFloat 3D textures #2046
Conversation
This is awesome, just had two comments on the code to review then I think it is good to go. |
Thanks @martinken for reviewing this. I will apply your comments today. I have one question though, this PR clearly reduces the GPU memory consumption (task manager screenshots); I was wondering about the CPU memory and how it is not getting that dramatic decrease? I checked the arrays feeding to |
I have no clue on memory issues in JS. It looks like you are saving about 200 MB on both CPU and GPU which seems reasonable to me and definitely a win. |
+1 |
If you want me to merge just let me know. LGTM |
@martinken Thanks again for your review, yes please merge it |
case VtkDataTypes.SHORT: | ||
return model.context.HALF_FLOAT; | ||
case VtkDataTypes.UNSIGNED_SHORT: | ||
return model.context.HALF_FLOAT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I spoke too soon. We need to think about this section right here. The issue is that float can fully represent short and unsigned short while half float cannot.
Short has 15 bits of precision versus about 11 in half float. Unsigned short has 16 bits versus half floats 11. So while we can stuff short and short and unsigned short into half float in webgl2 we are losing precision. So we are saving memory at the cost of precision. So it is a trade off. Any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I dug deeper into the half float standard, and according to Wikipedia section "Precision limitations on integer values" :
- Integers between 0 and 2048 can be exactly represented (and also between −2048 and 0)
- Integers between 2048 and 4096 round to a multiple of 2 (even number)
- Integers between 4096 and 8192 round to a multiple of 4
- Integers between 8192 and 16384 round to a multiple of 8
- Integers between 16384 and 32768 round to a multiple of 16
- Integers between 32768 and 65519 round to a multiple of 32
Since at the time we convert the buffer, we should know the min
and max
, one approach can be to convert by default if the min
and max
are in the range that can be exactly represented (-2048 to 2048), and be Optional (by flag, default false) if otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there is another extension EXT_texture_norm16, but it is totally unclear to me if it makes the texture filterable. Any comment on this extension to enable non-Float32 3D textures that are filterable? The detail of this extension is here, check Table 8.13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per that table it would be filterable (TF == Texture Filterable) so it would work. However I'm not sure what systems have that extension. I checked on Chrome/Firefox on windows and neither reported it
I like the idea of both checking the data range and having a setting. The setting could be on the volume mapper and named preferSizeOverAccuracy or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll add the preferSizeOverAccuracy
for this PR.
Regarding the EXT_texture_norm16
, I have it on my Chrome on Mac. I think it is GPU dependant, I checked mine on WebGL report website
Currently I'm working on a demo to utilize EXT_texture_norm16
, but I haven't had any luck so far, which is why I moved to half_float. However, I've begun a conversation with one of the developers of this extension, and I'll keep you updated, and hopefully create another PR later when the problem is resolved with EXT_texture_norm16
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since is no native support for 16 bit texture in webGL as far as I know, the options are
-
use float32 (memory inefficient)
-
use norm16Texture (not widely available)
- use half float (at the cost of inaccuracies in rendering, but almost widely available)
In WebGPU as I understand 16 bit texture is natively supported so vtk.js should not do that in webGPU. Otherwise, I can't think of any other solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you can put it under a flag to do half float when memory is the end goal, but i thought I added that here
fix(imageMapper): not exact half float should be under a flag #2786
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Int16 and Uint16 textures are supported by WebGL so I didn't understand why all this was needed
But I found out that even if they are supported but they are not filterable, which means that we can only use nearest neighbor interpolation
Do you think that using software interpolation would be a good idea?
It would cost 8 unfiltered texture sampling and 7 call to mix
but we don't have to do expensive operations to convert the volume, or loose memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not filterable correct. I mean I love to see software filtering on 16bits, but do you think doing 8 unfiltered texture sampling and 7 call to mix would be cheap? It might be, but if the fps is like 15 after I think we should let the user decide with some flags. But I love your idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this small shader in ShaderToy to benchmark trilinear sampling in the fragment shader
It looks like trilinear interpolation in software is around 8.5 times slower than in hardware, but I don't know how many samples are made per pixel in the volume mapper
I suppose that it depends on the options, for example if the shadows are enabled
@martinken I have edited the PR to include the Update on
|
@sedghi Let me know when you want me to take another review of it. The basic gist here all looks good, I will take another careful look at it once you think it is ready for it. |
@martinken This PR is ready for your review. Please go ahead. |
c11774c
to
e3d5bbf
Compare
LGTM, @sedghi can you rebase on master and squash the commits? Then we should be good to merge. |
I'll take a look at the additiveIntensity |
additive looks OK to me, I ran the volumemapperblendmodes example. Is there something you are seeing that doesn't look correct? |
Oddly enough we are counting on webgl2 supporting linear float textures and yet looking at the spec I don't think it is required. SO we are just getting lucky I guess. |
30fa1ad
to
a6024d2
Compare
@martinken I did rebase and squash. Are you running the The problem I can see for the additiveBlend is that it renders empty compared to without the |
Ahh OK let me give that a try |
The issue is the ipScalarRange code. It is written to go from a range of 0 to 1 and when using half float textures the texture values are coming back with a larger range (the actual range of the data). The trick is that due to texture limitations we have to encode data into many texture formats, so the texture class has a shift/scale factor it computes that reflects how the texture map values map to the original dataset. (see line 1217 of Texture). When using halfFloat those values have to change. I pushed a commit with a fix for it that also makes it much faster when changing values. |
I just happened to be in that code yesterday on the WebGPU side so the timing was fortunate :-) |
Please give my change a look/test and see if it works for you/makes sense. |
ipScalarRange is a 0-1 value in texture values which with different texture formats may require different scaling/offsets. This commit fixes it's use for halfFloat textures and also changes how it is implemented to be done with uniforms which are quick to change, instead of changing the shader program which is expensive.
40f8265
to
eb116f5
Compare
@martinken Thank you for correcting the additive blend and providing such a thorough explanation of the root cause of the issue; I really appreciate it. |
🎉 This PR is included in version 19.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Recently half float precision for 2D textures was added, this PR will try to add it for the 3D textures too.
Data used is a full body CT from TCIA (download .vti from here).
run
npm run example -- VolumeMapperBlendModes
with the dataBefore
2021-08-27.17-21-22.mp4
Chrome task manager before
After
2021-08-27.17-21-22.mp4
Chrome task manager after
PR and Code Checklist
npm run reformat
to have correctly formatted codeContext
Changes
Results
Testing
CC'ing @swederik