remove subset adjustment from slice shader #224
Conversation
toloudis
requested review from
meganrm,
rugeli and
frasercl
and removed request for
a team
| zSlider.max = `${totalZSlices}`; | ||
| zInput.max = `${totalZSlices}`; | ||
| zSlider.max = `${totalZSlices - 1}`; | ||
| zInput.max = `${totalZSlices - 1}`; |
There was a problem hiding this comment.
this is just a bonus fix to fix the slider range in the testbed viewer
| @@ -87,7 +86,6 @@ void main() { | |||
| vec4 pos = vec4(vUv, | |||
| (SLICES==1.0 && Z_SLICE==0) ? 0.0 : float(Z_SLICE) / (SLICES - 1.0), | |||
| 0.0); | |||
| pos.xyz = (pos.xyz - SUBSET_OFFSET) / SUBSET_SCALE; | |||
There was a problem hiding this comment.
removing this line is the bug fix. For example: when pos.z = 1.0, we could get into a situation where this math was doing (1.0 - 0.9666) / 0.0333 which is slightly greater than 1. Then later there is bounds checking code which needs to ensure that pos is never greater than 1.
| const { normRegionSize, normRegionOffset } = this.volume; | ||
| const offsetToCenter = normRegionSize.clone().divideScalar(2).add(normRegionOffset).subScalar(0.5); | ||
| const bmin = bounds.bmin.clone().sub(offsetToCenter).divide(normRegionSize).clampScalar(-0.5, 0.5); | ||
| const bmax = bounds.bmax.clone().sub(offsetToCenter).divide(normRegionSize).clampScalar(-0.5, 0.5); | ||
|
|
||
| this.setUniform("AABB_CLIP_MIN", bounds.bmin); | ||
| this.setUniform("AABB_CLIP_MAX", bounds.bmax); | ||
| this.setUniform("AABB_CLIP_MIN", bmin); | ||
| this.setUniform("AABB_CLIP_MAX", bmax); |
There was a problem hiding this comment.
This looks like the right thing to do for bounding box clipping, but there might still be more to do for scaling - see https://github.com/allen-cell-animated/volume-viewer/blob/main/src/RayMarchedAtlasVolume.ts#L86. Though https://github.com/allen-cell-animated/volume-viewer/blob/main/src/Atlas2DSlice.ts#L102 might already be doing the right thing for that?
Fixing #223
This extra adjustment was incurring floating point errors which make the last z slice go out of bounds.
Rather than add a bounds epsilon, I found that removing this code seemed to fix the bug without causing any visual discrepancies.
@frasercl would love your insight as to whether this is totally safe. I think I tested a variety of files but not sure I know why it was there in the first place.