Skip to content

Conversation

@daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Dec 1, 2025

  • Fix plane scaling and datasets where one voxel size dimension is much smaller than the other ones
    • The planes were scaled to be too large*
    • For datasets where one voxel dimension is much smaller than the rest, for example 10, 10, 0.1 or 1, 100, 100, the vertex shader bucket border optimization (plane_subdivision) caused issues, because the number of subdivisions was not enough to cover all bucket borders. This resulted in certain planes being rendered only partially for certain zoom values (usually worst right before rendering the next worse zoom step)

*compare the scaling code before the ortho viewport rotation PR

URL of deployed dev instance (used for testing):

Steps to test:

Issues:


(Please delete unneeded items, merge only when none are left open)

  • Added changelog entry (create a $PR_NUMBER.md file in unreleased_changes or use ./tools/create-changelog-entry.py)
  • Removed dev-only changes like prints and application.conf edits
  • Considered common edge cases

@daniel-wer daniel-wer self-assigned this Dec 1, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 1, 2025

📝 Walkthrough

Walkthrough

This PR switches plane/world scaling to use base-voxel units, threads base-voxel factors through flycam code and tests, adds a vertex→fragment flat varying plus a vertex early-return guard to disable a bucket-border vertex optimization when unsafe, updates a wording in scaleinfo, and adds a changelog entry and test dataset for extreme anisotropy.

Changes

Cohort / File(s) Summary
Base-voxel scaling & flycam
frontend/javascripts/viewer/geometries/plane.ts, frontend/javascripts/viewer/model/scaleinfo.ts, frontend/javascripts/viewer/model/accessors/flycam_accessor.ts, frontend/javascripts/viewer/model/reducers/flycam_reducer.ts, frontend/javascripts/viewer/model/sagas/flycam_info_cache_saga.ts
Replace uses of raw dataset scale factor/vector with base-voxel units/factors: plane.ts uses getBaseVoxelInUnit to build plane scale; flycam accessor/reducer/saga now accept/propagate VoxelSize and compute matrices from base-voxel factors; scaleinfo.ts has a comment wording change only.
Shader varying & optimization guard
frontend/javascripts/viewer/shaders/main_data_shaders.glsl.ts, frontend/javascripts/viewer/shaders/texture_access.glsl.ts
Add flat varying useBucketBorderVertexOptimization (vertex out / fragment in). Vertex shader initializes and conditionally disables it (and early-returns) when rotations/transforms are present or subdivision/viewport coverage is insufficient; texture access now uses this flag instead of prior rotation/orthogonality checks.
Tests & test data
frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts, frontend/javascripts/test/model/accessors/flycam_accessors.spec.ts
Add test-extreme-anisotropy dataset and view override; update unit test to use VoxelSize (object with factor/unit) and pass voxelSize to dummy flycam matrix factory.
Changelog
unreleased_changes/9114.md
Add unreleased note documenting fix for rendering issues in extremely anisotropic datasets.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Files/areas that may require extra attention:
    • Shader changes: verify flat varying declaration, correct propagation, and that the vertex early-return does not discard needed geometry or affect draw ordering.
    • Viewport coverage calculation and the condition that disables the optimization across camera modes (orthogonal vs flycam) and subdivision settings.
    • Flycam API/signature changes: resetMatrix and _getDummyFlycamMatrix now accept VoxelSize — ensure no remaining callsites expect the old Vector3/factor shape.
    • Interaction of base-voxel scaling with other world-space math (plane scaling, camera matrices, and any code assuming dataset-scale-factor semantics).
    • Tests: updated unit tests and new dataset entry should reflect actual dataset metadata (voxel units/factors).

Possibly related PRs

Suggested labels

frontend, bug

Suggested reviewers

  • philippotto
  • fm3

Poem

🐇 I hopped through voxels neat and spry,
Counting base units as I fly.
A shader guard keeps borders true,
No jagged seams to bother you.
Happy pixels — nibble a carrot, bye! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main fix: addressing rendering issues in highly anisotropic datasets through plane scaling corrections.
Description check ✅ Passed The description provides clear context about the problem (plane scaling and bucket border optimization issues in anisotropic datasets), references specific examples, includes testing steps, and links to supporting materials.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-plane-scaling-and-extreme-anisotropic-datasets

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 051cbfd and 4f51bd1.

⛔ Files ignored due to path filters (1)
  • frontend/javascripts/test/screenshots/test-extreme-anisotropy.png is excluded by !**/*.png
📒 Files selected for processing (5)
  • frontend/javascripts/test/model/accessors/flycam_accessors.spec.ts (3 hunks)
  • frontend/javascripts/viewer/geometries/plane.ts (2 hunks)
  • frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (3 hunks)
  • frontend/javascripts/viewer/model/reducers/flycam_reducer.ts (4 hunks)
  • frontend/javascripts/viewer/model/sagas/flycam_info_cache_saga.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/javascripts/viewer/geometries/plane.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.

Applied to files:

  • frontend/javascripts/test/model/accessors/flycam_accessors.spec.ts
  • frontend/javascripts/viewer/model/sagas/flycam_info_cache_saga.ts
  • frontend/javascripts/viewer/model/accessors/flycam_accessor.ts
  • frontend/javascripts/viewer/model/reducers/flycam_reducer.ts
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.

Applied to files:

  • frontend/javascripts/test/model/accessors/flycam_accessors.spec.ts
  • frontend/javascripts/viewer/model/accessors/flycam_accessor.ts
  • frontend/javascripts/viewer/model/reducers/flycam_reducer.ts
📚 Learning: 2025-05-07T06:17:32.810Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8602
File: frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts:365-366
Timestamp: 2025-05-07T06:17:32.810Z
Learning: The parameter in applyVoxelMap was renamed from `sliceCount` to `sliceOffset` to better reflect its purpose, but this doesn't affect existing call sites since JavaScript/TypeScript function calls are position-based.

Applied to files:

  • frontend/javascripts/test/model/accessors/flycam_accessors.spec.ts
  • frontend/javascripts/viewer/model/accessors/flycam_accessor.ts
  • frontend/javascripts/viewer/model/reducers/flycam_reducer.ts
📚 Learning: 2025-04-01T09:45:17.527Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8485
File: frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts:384-392
Timestamp: 2025-04-01T09:45:17.527Z
Learning: The function `isRotationAndMirrorMaybeOnly` in the dataset_layer_transformation_accessor.ts is intentionally designed to allow mirroring transformations (negative scale values). It uses the length comparison (`scale.length() === NON_SCALED_VECTOR.length()`) rather than component equality to permit mirrored axes while ensuring the overall scale magnitude remains the same.

Applied to files:

  • frontend/javascripts/test/model/accessors/flycam_accessors.spec.ts
  • frontend/javascripts/viewer/model/accessors/flycam_accessor.ts
  • frontend/javascripts/viewer/model/reducers/flycam_reducer.ts
📚 Learning: 2024-11-22T17:17:39.914Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts:125-125
Timestamp: 2024-11-22T17:17:39.914Z
Learning: In the `createNode` function within `skeletontracing_reducer_helpers.ts`, the property `resolution` is intentionally kept even when the parameter is renamed to `mag`.

Applied to files:

  • frontend/javascripts/test/model/accessors/flycam_accessors.spec.ts
  • frontend/javascripts/viewer/model/reducers/flycam_reducer.ts
📚 Learning: 2025-07-22T12:37:09.684Z
Learnt from: daniel-wer
Repo: scalableminds/webknossos PR: 8787
File: frontend/javascripts/libs/UpdatableTexture.ts:75-79
Timestamp: 2025-07-22T12:37:09.684Z
Learning: In Three.js r163 and later versions (including r169), the WebGLUtils constructor signature was changed to remove the capabilities argument. The correct constructor signature is `new WebGLUtils(gl, extensions)` with only two parameters. This change was part of the WebGL 1 support deprecation in r163.

Applied to files:

  • frontend/javascripts/viewer/model/reducers/flycam_reducer.ts
🧬 Code graph analysis (2)
frontend/javascripts/test/model/accessors/flycam_accessors.spec.ts (2)
frontend/javascripts/types/api_types.ts (1)
  • VoxelSize (144-147)
frontend/javascripts/viewer/constants.ts (1)
  • Identity4x4 (387-387)
frontend/javascripts/viewer/model/sagas/flycam_info_cache_saga.ts (1)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
  • _getDummyFlycamMatrix (200-203)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: backend-tests
  • GitHub Check: build-smoketest-push
  • GitHub Check: frontend-tests
🔇 Additional comments (7)
frontend/javascripts/viewer/model/sagas/flycam_info_cache_saga.ts (1)

103-103: LGTM!

The change correctly passes the full VoxelSize object instead of just the numeric factor array, aligning with the updated _getDummyFlycamMatrix signature that now accepts VoxelSize for proper anisotropic scaling.

frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)

200-203: LGTM! Core refactoring for anisotropic dataset support.

The signature change from Vector3 to VoxelSize and the switch to getBaseVoxelFactorsInUnit are central to fixing the plane scaling issues described in the PR. This enables proper handling of datasets where one voxel-size dimension is much smaller than the others.

frontend/javascripts/test/model/accessors/flycam_accessors.spec.ts (2)

86-86: LGTM! Test properly updated for VoxelSize-based API.

The test correctly constructs a VoxelSize object with both factor and unit properties, aligning with the updated function signatures. The expected zoom values remain unchanged, confirming backward compatibility of the computation logic.


118-123: LGTM! Correct parameter usage for updated APIs.

The test appropriately passes voxelSize.factor (numeric array) to _getMaximumZoomForAllMags and the full voxelSize object to _getDummyFlycamMatrix, matching their respective signatures.

frontend/javascripts/viewer/model/reducers/flycam_reducer.ts (3)

104-115: LGTM! Consistent refactoring to VoxelSize-based scaling.

The resetMatrix function now accepts VoxelSize and uses getBaseVoxelFactorsInUnit to compute the scale matrix. This change enables proper base-voxel scaling for anisotropic datasets while correctly preserving the camera position during rotation resets.


178-178: LGTM! Call site updated correctly.

The call to resetMatrix now passes the full state.dataset.dataSource.scale (VoxelSize) instead of just the factor, matching the updated function signature.


203-203: LGTM! Call site updated correctly.

The call to resetMatrix in the SET_DATASET reducer now passes the full action.dataset.dataSource.scale (VoxelSize) instead of just the factor, matching the updated function signature.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

excellent 👍 extra kudos for the regression test 🥇

@MichaelBuessemeyer
Copy link
Contributor

MichaelBuessemeyer commented Dec 5, 2025

Awesome work! Thanks you two for fixing this 🚀 🙏

We just noticed this bug in another dataset again. Would be awesome to get this merged starting next week 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants