Skip to content

fix(sliceview): use different bounds to compute chunk positions #1035

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.

Sign up for GitHub

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

Open
minnerbe wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(sliceview): use different bounds to compute chunk positions #1035

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3af122e
fix(sliceview): use different bounds to compute chunk positions
minnerbe Jun 9, 2026
55fe839
Add regression test
minnerbe Jun 10, 2026
a3b0b75
Only clamp for non-display dims
minnerbe Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/sliceview/base.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,11 @@
*/

import { describe, it, expect } from "vitest";
import type { ProjectionParameters } from "#src/projection_parameters.js";
import type { TransformedSource } from "#src/sliceview/base.js";
import {
estimateSliceAreaPerChunk,
forEachVisibleVolumetricChunk,
getNearIsotropicBlockSize,
} from "#src/sliceview/base.js";
import { ChunkLayout } from "#src/sliceview/chunk_layout.js";
Expand Down Expand Up @@ -200,3 +203,61 @@ describe("estimateSliceAreaPerChunk", () => {
}
});
});

describe("forEachVisibleVolumetricChunk", () => {
it("does not clamp zeroed display-dim positions to the chunk origin", () => {
// `xy` slice view of a rank-3 volume whose origin is *not* at (0, 0, 0)
const tsource = {
source: {
spec: {
rank: 3,
chunkDataSize: Uint32Array.of(1024, 1024, 1),
// Stack origin at voxel (64056, 33042, 20) -> nonzero chunk bounds.
lowerChunkBound: Float32Array.of(62, 32, 20),
upperChunkBound: Float32Array.of(64, 34, 21),
},
},
layerRank: 3,
// The following data is prepared as would be done by
// `getVolumetricTransformedSources`: display-dim rows (x, y) are zeroed and
// z maps identically from global z with no translation.
fixedLayerToChunkTransform: Float32Array.of(
0,
0,
0,
0,
0,
0,
0,
0,
1,
0,
0,
0,
),
// Display dims (x, y) get infinite clip bounds; only z is finite.
nonDisplayLowerClipBound: Float32Array.of(-Infinity, -Infinity, 20),
nonDisplayUpperClipBound: Float32Array.of(Infinity, Infinity, 21),
chunkLayout: new ChunkLayout(vec3.fromValues(1, 1, 1), mat4.create(), 3),
lowerChunkDisplayBound: vec3.fromValues(62, 32, 20),
upperChunkDisplayBound: vec3.fromValues(64, 34, 21),
chunkDisplayDimensionIndices: [0, 1, 2],
curPositionInChunks: new Float32Array(3),
// Sentinel so an early return (source excluded) can't masquerade as a pass.
fixedPositionWithinChunk: Uint32Array.of(999, 999, 999),
} as unknown as TransformedSource;

forEachVisibleVolumetricChunk(
{
// x/y are irrelevant (transform rows zeroed); z is at slice 20.
globalPosition: Float32Array.of(70000, 40000, 20),
viewProjectionMat: mat4.create(),
} as unknown as ProjectionParameters,
new Float32Array(0),
tsource,
() => {},
);

expect(Array.from(tsource.fixedPositionWithinChunk)).toEqual([0, 0, 0]);
});
});
20 changes: 13 additions & 7 deletions src/sliceview/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,7 @@ function updateFixedCurPositionInChunks<
): boolean {
const { curPositionInChunks, fixedPositionWithinChunk } = tsource;
const { nonDisplayLowerClipBound, nonDisplayUpperClipBound } = tsource;
const { rank, chunkDataSize, lowerChunkBound, upperChunkBound } =
tsource.source.spec;
const { rank, chunkDataSize } = tsource.source.spec;
if (
!getChunkPositionFromCombinedGlobalLocalPositions(
curPositionInChunks,
Expand Down Expand Up @@ -219,12 +218,19 @@ function updateFixedCurPositionInChunks<
return false;
}
const chunkSize = chunkDataSize[chunkDim];
// Given that clip bounds are already tested above, clamp chunk index to its
// bounds, to ensure floating-point imprecision does not result in an
// out-of-bounds index.
// Given that clip bounds are already tested above, clamp the chunk index to
// the range implied by those clip bounds, to ensure floating-point
// imprecision does not result in an out-of-bounds index. The clip bounds
// are used because they are expressed in the same coordinate frame as `x`.
const lowerChunkLimit = Math.floor(
nonDisplayLowerClipBound[chunkDim] / chunkSize,
);
const upperChunkLimit = Math.ceil(
nonDisplayUpperClipBound[chunkDim] / chunkSize,
);
const chunk = (curPositionInChunks[chunkDim] = Math.min(

@seankmartin seankmartin Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for raising this and making the example which helps a lot. I am a bit concerned this might not be the right fix though. I understand from the test point of view the fix. But in practice I think the non display clip bounds end up being positive and negative infinity for the dims being clamped here and then this reduces to chunk = curPositionInChunks[chunkDim] = Math.floor(x / chunkSize). So maybe we instead just need to detect cases to not use clip bounds, or try to fix these bounds for the issue you are encountering? I'm also struggling a bit with this conceptually because I thought the chunk index would be for display dimensions so I don't really understand why we'd clamp against the non display bounds.

@minnerbe minnerbe Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay! I agree, it's a bit hard to wrap my head around this conceptually. As far as I understand right now, updateFixedCurPositionInChunks' responsibility is to process the non-display dims only. The display dims have a curPositionInChunks[chunkDim] = 0 placeholder (the display rows of fixedLayerToChunkTransform are zeroed in frontend.ts:1033) that the pre-a045804 code preserved, but the current code might clamp to a non-zero value, which causes fixedPositionWithinChunk to wrap. That only came up in the render backend because it sets a non-zero lowerVoxelBound.

Having understood this, maybe a better fix would be to just iterate over the non-display dimensions in the first place? This would allow us to clamp against {lower,upper}ChunkBound again. I've pushed that for the sake of discussion (I'm happy to squash/rebase later).

upperChunkBound[chunkDim] - 1,
Math.max(lowerChunkBound[chunkDim], Math.floor(x / chunkSize)),
upperChunkLimit - 1,
Math.max(lowerChunkLimit, Math.floor(x / chunkSize)),
));
fixedPositionWithinChunk[chunkDim] = x - chunk * chunkSize;
}
Expand Down
Loading