fix(sliceview): use different bounds to compute chunk positions - #1035
fix(sliceview): use different bounds to compute chunk positions#1035minnerbe wants to merge 3 commits into
Conversation
It seems that spec.{lower,upper}*ChunkBound is in global coordinates,
whereas curPositionInChunks is zeroed in the display dimensions. Due to
this mismatch, clamping the chunk index produces wrong results and the
viewing area stays blank. Using nonDisplay{lower,upper}ClipBound to
compute chunk bounds fixes that.
|
@minnerbe do you have a publicly shareable example? We should add a regression test for this. Still need to analyze the issue. |
|
Thanks for the fast reply, @chrisj! We currently don't have a public-facing render instance that could serve example data. If necessary we could set up a temporary instance, though. It also seems that chunked data like zarr won't trigger this behavior since origins other than (0, 0) are represented by coordinate transformations. This is different in render, where they are represented by a nonzero I added a first version of a regression test that sets up the same state as in the example I tested. It fails before the fix and passes after. Unfortunately, testing lower ( |
|
In the last week, @trautmane kindly put together a minimal render webserver with a small example dataset. You can see a neuroglancer view here. This example fails to show in the current ng deployment, but renders in our internal ng instance that includes the fix from this PR (see screenshot below). 
|
| const upperChunkLimit = Math.ceil( | ||
| nonDisplayUpperClipBound[chunkDim] / chunkSize, | ||
| ); | ||
| const chunk = (curPositionInChunks[chunkDim] = Math.min( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
3 participants
When using the render backend, stacks sometimes don't have their origin at (0, 0). Those stacks would render normally on 2.40.1, but produced a blank display on 2.41.2.
Claude helped me figure out that this is caused by the clamping introduced by a045804 in
updateFixedCurPositionInChunks. There,curPositionInChunks(which is zeroed in the display dimensions) is clamped againstspec.{lower,upper}ChunkBound(which is non-zero in these situations), leading to a negative chunk offset. This PR suggests to clamp against bounds computed fromnonDisplay{Lower,Upper}ClipBound, which fixes the problem we had.I'm happy for any feedback, especially if this conceptually isn't the right fix.