Skip to content

Conversation

@daljit46
Copy link
Member

@daljit46 daljit46 commented Oct 1, 2025

This small change fixes #2319.
After the discussion this morning with @bjeurissen, I decided to investigate this long-standing issue in the viewer. So this wasn't really a driver issue as discussed in the relevant issue, but an error in our code.
After tinkering with OpenGL code a bit, I noticed that the undo/redo shader in undoentry.cpp was using an integer sampler to read the values from the texture.
Our texture data type is GL_R8, which is an 8-bit normalised fixed-point format. It needs a float sampler, because integer samplers are for integer formats like GL_R8I.

@MRtrix3/mrtrix3-devs should we include this fix in 3.0.8?

@github-actions
Copy link

github-actions bot commented Oct 1, 2025

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie
Copy link
Member

If it's a bug fix, then it should be targeting master, whether it gets merged in time for 3.0.8 or not.

I worry here that the fix is being applied at the wrong end & in the wrong direction. The ROI tool deals exclusively with bitwise data and no interpolation. It would make a lot more sense to be using 8-bit integer data. It's more likely that it's the upstream texture data type that's the bug, not the downstream sampler.

@daljit46
Copy link
Member Author

daljit46 commented Oct 2, 2025

If it's a bug fix, then it should be targeting master, whether it gets merged in time for 3.0.8 or not.

Will rebase this on top of master.

I worry here that the fix is being applied at the wrong end & in the wrong direction. The ROI tool deals exclusively with bitwise data and no interpolation. It would make a lot more sense to be using 8-bit integer data. It's more likely that it's the upstream texture data type that's the bug, not the downstream sampler.

We could use an integer texture, but more changes would be needed.
With the current proposal, since we're storing 0 or 1, in the shader (texelFetch()) we'll either get 0.0 or 1/255, which will get quantised to 0 or 1 as expected once we read back to CPU. So I don't think there's any danger in using a fixed-point texture here.
If we decide to change the texture format to an integer one, I think we may need to think about the interaction of the ROI texture code with the rest of the rendering code, which operates assuming non-integer samplers. I don't think the changeset should be too big, so if we want this for better code clarity and intent, I'm happy to do that.

@Lestropie
Copy link
Member

Given that the various tools operate sequentially and independently with their own shaders, I would have thought it not too difficult, but I'm admittedly highly out of practise with the GUI code. Maybe have a dig at it, but if you encounter anything that looks like it could gobble up a lot of time revert back to the one-line fix?

Use an unsigned integer 3D texture for the ROI (GL_R8UI) to store a
0/1 mask without normalization. Previously, we were using an integer
sampler with a GL_R8 texture leading to erraneous behaviour on MacOS. 
Any future code interacting with the ROI texture must avoid trying to 
enable filtering and use integer sampler to read from it.
@daljit46 daljit46 changed the base branch from dev to master October 2, 2025 21:13
@daljit46
Copy link
Member Author

daljit46 commented Oct 2, 2025

This is now targeting master.

I changed the format for the ROI texture from GL_R8 to GL_R8UI. The changes are still minimal. A noteworthy change is in slice.cpp, where the slice shader must sample it with an integer sampler and convert that integer into a float mask for the rest of the float-based rendering pipeline. So I modified the fragment shader to use usampler3D for ROI volumes and sampler3D for normal float volumes. Future code needs to take care of using the correct (integer) sampler when interacting with the ROI texture.

@bjeurissen bjeurissen mentioned this pull request Oct 4, 2025
13 tasks
@jdtournier jdtournier added this pull request to the merge queue Oct 8, 2025
Merged via the queue into master with commit 1bb646b Oct 8, 2025
5 checks passed
@jdtournier jdtournier deleted the fix_roi_macos branch October 8, 2025 09:27
@jdtournier jdtournier mentioned this pull request Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mrview: ROI edits deleted after two elements drawn

5 participants