Use datastore segment index when loading ad-hoc meshes #8922
Conversation
This PR adds two new camera perspectives for animation jobs: - static camera looking at XY plane (top-down view on the stack) - static, isometric camera looking a the corner between XY, XZ, YZ plane - CI should be enough for the frontend, backend changes - Otherwise see worker PR - [ ] Merge related [worker PR first](scalableminds/voxelytics#4279) - None ------ (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`) - [ ] Added migration guide entry if applicable (edit the same file as for the changelog) - [x] Updated [documentation](../blob/master/docs) if applicable - [ ] Adapted [wk-libs python client](https://github.com/scalableminds/webknossos-libs/tree/master/webknossos/webknossos/client) if relevant API parts change - [ ] Removed dev-only changes like prints and application.conf edits - [ ] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) - [ ] Needs datastore update after deployment
📝 WalkthroughWalkthroughRoute frontend segment-statistics and ad-hoc mesh calls through a new LayerSourceInfo type using getDataOrTracingStoreUrl; replace raw requestUrl parameters with LayerSourceInfo, add hasSegmentIndexInDataStoreCached, propagate LayerSourceInfo through mesh sagas, and update backend ad-hoc mesh handling to use segment-index-driven flow and parameterless cuboid accessors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)frontend/javascripts/viewer/view/context_menu.tsx (2)
⏰ 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)
🔇 Additional comments (3)
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. Comment |
fm3
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
362-367: Reuse the resolved data layer name for the datastore URL.Keep this consistent with the check above to avoid mismatches.
Apply this diff:
- const dataStoreUrl = `${datastoreUrl}/data/datasets/${datasetId}/layers/${ - layer.fallbackLayer != null ? layer.fallbackLayer : layer.name - }`; + const dataStoreUrl = `${datastoreUrl}/data/datasets/${datasetId}/layers/${dataLayerNameForRequests}`;frontend/javascripts/admin/rest_api.ts (1)
1932-1955: Omit null fields in getBucketPositionsForAdHocMesh payload
ConvertadditionalCoordinatesandmappingNametoundefinedwhen null to prevent sendingnullin the JSON payload:data: { cubeSize, mag, - additionalCoordinates, - mappingName, + additionalCoordinates: additionalCoordinates || undefined, + mappingName: mappingName ?? undefined, },Route
/segmentIndex/:segmentIdexists in both datastore and tracingstore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/rest_api.ts(2 hunks)frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/javascripts/admin/rest_api.ts (3)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/types/api_types.ts (1)
AdditionalCoordinate(33-33)frontend/javascripts/admin/api/token.ts (1)
doWithToken(39-74)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
frontend/javascripts/admin/rest_api.ts (2)
hasSegmentIndexInDataStoreCached(858-858)getBucketPositionsForAdHocMesh(1931-1956)
⏰ 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: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (7)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (7)
4-6: Import looks good.
326-328: Ensure dataset is always available here.If dataset could be null during early loads or transitions, this will throw. You likely rely on ensureWkReady, but please confirm.
354-358: Gating logic for volume tracing segment-index usage looks correct.Please confirm the backend returns a boolean for hasSegmentIndex; otherwise a truthy object here would incorrectly enable the path.
359-361: Combining the two conditions is sensible.
416-424: Signature update reads well.
426-433: Correctly threads new params.No issues. Sorting by distance after scaling is correct.
369-378: LGTM — tracingstore exposes the segmentIndex endpoint and accepts mappingName. Confirmed: POST /volume/:tracingId/segmentIndex/:segmentId is defined (webknossos-tracingstore/conf/tracingstore.latest.routes) and TSRemoteDatastoreClient posts mappingName in GetMultipleSegmentIndexParameters (app/.../TSRemoteDatastoreClient.scala).
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/rest_api.ts(2 hunks)frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#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/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
📚 Learning: 2024-11-22T17:19:07.947Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx:433-434
Timestamp: 2024-11-22T17:19:07.947Z
Learning: In the codebase, certain usages of `segmentationLayer.resolutions` are intentionally retained and should not be changed to `segmentationLayer.mags` during refactoring.
Applied to files:
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
📚 Learning: 2025-03-14T09:56:39.972Z
Learnt from: philippotto
PR: scalableminds/webknossos#8448
File: frontend/javascripts/oxalis/model/sagas/flycam_info_cache_saga.ts:126-127
Timestamp: 2025-03-14T09:56:39.972Z
Learning: In the WebKnossos codebase, the `getComputeFunction` uses memoization with both `_.memoize` and `memoizeOne`, ensuring that the same reference is returned for identical inputs, making reference equality checks (`!==`) sufficient and more efficient than structural equality checks.
Applied to files:
frontend/javascripts/admin/rest_api.ts
🧬 Code graph analysis (2)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
frontend/javascripts/admin/rest_api.ts (2)
hasSegmentIndexInDataStoreCached(858-860)getBucketPositionsForAdHocMesh(1933-1958)
frontend/javascripts/admin/rest_api.ts (3)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/types/api_types.ts (1)
AdditionalCoordinate(33-33)frontend/javascripts/admin/api/token.ts (1)
doWithToken(39-74)
⏰ 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: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
|
@fm3 can you have a look whether this is what you imagined? afterwards I'll request review :) |
|
Yes, looking good! To simplify, you can rely on the fact that there cannot be a fallback layer if volumeTracing == null. So you could probably replace maybeFallbackLayerName by just using …if my understanding is correct. Not sure if the frontend does any magic with the layer handling. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
362-365: Build dataStoreUrl with the same fallback-aware name used for requests.This aligns segment-index and mesh requests with the actual layer served by the datastore (matches maybeLoadMeshChunk).
- const dataStoreUrl = `${dataStoreHost}/data/datasets/${datasetId}/layers/${layer.name}`; - const tracingStoreUrl = `${tracingStoreHost}/tracings/volume/${layer.name}`; - const requestUrl = useDataStore ? dataStoreUrl : tracingStoreUrl; + const dataStoreUrl = `${dataStoreHost}/data/datasets/${datasetId}/layers/${dataLayerNameForRequests}`; + const tracingStoreUrl = `${tracingStoreHost}/tracings/volume/${layer.name}`; + const requestUrl = useDataStore ? dataStoreUrl : tracingStoreUrl;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#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/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
📚 Learning: 2024-11-22T17:19:07.947Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx:433-434
Timestamp: 2024-11-22T17:19:07.947Z
Learning: In the codebase, certain usages of `segmentationLayer.resolutions` are intentionally retained and should not be changed to `segmentationLayer.mags` during refactoring.
Applied to files:
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
🧬 Code graph analysis (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
frontend/javascripts/admin/rest_api.ts (2)
hasSegmentIndexInDataStoreCached(858-860)getBucketPositionsForAdHocMesh(1933-1958)
⏰ 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 (6)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (6)
4-4: Import looks correct.Brings in the memoized datastore segment-index check used below.
325-327: LGTM: selecting dataset context once.Grabs datasetId and dataStoreHost for later URL construction.
368-375: LGTM: requestUrl + mappingName threaded to segment-index positions.Matches the updated REST API signature and preserves mapping context.
342-351: Use fallback-aware layer name and gate static-layer seg-index check by useDataStore.Without resolving the fallback name, the datastore check can miss an existing segment index; and without gating, a forced useDataStore=false may later cause a tracing-store call to /segmentIndex for a static layer (404). Apply:
- let isStaticSegmentationLayerWithSegmentIndex = false; - - if (volumeTracing == null) { - isStaticSegmentationLayerWithSegmentIndex = yield* call( - hasSegmentIndexInDataStoreCached, - dataset.dataStore.url, - dataset.id, - layer.name, - ); - } + let isStaticSegmentationLayerWithSegmentIndex = false; + const dataLayerNameForRequests = + layer.fallbackLayer != null ? layer.fallbackLayer : layer.name; + if (volumeTracing == null && useDataStore) { + isStaticSegmentationLayerWithSegmentIndex = yield* call( + hasSegmentIndexInDataStoreCached, + dataset.dataStore.url, + dataset.id, + dataLayerNameForRequests, + ); + }
359-361: Ensure we never query tracing-store segment index for static layers.Also gate the derived flag on useDataStore to keep requestUrl and endpoint in sync.
- const usePositionsFromSegmentIndex = - usePositionsFromSegmentIndexForVolumeTracing || isStaticSegmentationLayerWithSegmentIndex; + const usePositionsFromSegmentIndex = + usePositionsFromSegmentIndexForVolumeTracing || + (useDataStore && isStaticSegmentationLayerWithSegmentIndex);
415-431: All call sites have been updated to the new signature.
|
@philippotto can you have a look at the current frontend changes in this PR? |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/BinaryDataController.scala (1)
224-235: Add central cuboid validation in AdHocMeshService.requestAdHocMesh
Guard incoming cuboid dimensions and alignment before meshing:--- a/app/com/scalableminds/webknossos/datastore/services/mesh/AdHocMeshService.scala +++ b/app/com/scalableminds/webknossos/datastore/services/mesh/AdHocMeshService.scala @@ def requestAdHocMesh(request: AdHocMeshRequest)(implicit tc: TokenContext): Fox[(Array[Float], List[Int])] = { - request.dataLayer.elementClass match { + for { + _ <- Fox.fromBool( + request.cuboid.width <= DataLayer.bucketLength + 1 && + request.cuboid.height <= DataLayer.bucketLength + 1 && + request.cuboid.depth <= DataLayer.bucketLength + 1 && + request.cuboid.topLeft == request.cuboid.topLeft.topLeft + ) ?~> "adhoc.cuboid.invalid" + result <- request.dataLayer.elementClass match { + // existing mesh logic… + } + } yield resultThis single change protects all callers (BinaryDataController, LegacyController, VolumeTracingController, EditableMappingService) from oversized or misaligned cuboids.
🧹 Nitpick comments (4)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)
399-413: AdHocMeshRequest: pass through additionalCoordinates for parity with VolumeTracingServiceSwitch to
request.cuboidlooks correct. To support datasets with additional axes, also forwardrequest.additionalCoordinateshere (as is done in VolumeTracingService). Otherwise ad‑hoc meshes could be looked up in the wrong coordinate slice.val adHocMeshRequest = AdHocMeshRequest( dataSourceId = None, dataLayer = editableMappingLayer, cuboid = request.cuboid, segmentId = request.segmentId, voxelSizeFactor = request.voxelSizeFactorInUnit, tokenContext = tc, mapping = None, mappingType = None, - findNeighbors = request.findNeighbors + additionalCoordinates = request.additionalCoordinates, + findNeighbors = request.findNeighbors )webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (3)
70-86: Good fallback strategy: segment-index path first, neighbor logic otherwise.Branching by availability of a segment index is sensible. Please also log which path was taken and basic counts (e.g., segments, buckets) to aid ops triage.
114-139: Consider bounded parallelism for many buckets.
Fox.serialCombined(targetMagPositions)processes buckets sequentially; large segments could be slow. If available, prefer a bounded-parallel variant (e.g., fixed parallelism from config) to improve throughput without overloading the actor pool.
114-139: Nit: variable name implies neighbors but they’re disabled.
vertexChunksWithNeighborsholds(Array[Float], List[Int])butfindNeighbors = false. Consider renaming tovertexChunksfor clarity.- vertexChunksWithNeighbors: List[(Array[Float], List[Int])] <- Fox.serialCombined(targetMagPositions) { + vertexChunks: List[(Array[Float], List[Int])] <- Fox.serialCombined(targetMagPositions) { targetMagPosition => val adHocMeshRequest = AdHocMeshRequest( ... findNeighbors = false, ) adHocMeshService.requestAdHocMeshViaActor(adHocMeshRequest) } - allVertices = vertexChunksWithNeighbors.map(_._1) + allVertices = vertexChunks.map(_._1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/BinaryDataController.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala(4 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (4)
cuboid(24-24)cuboid(37-40)cuboid(59-61)cuboid(73-76)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/BinaryDataController.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (4)
cuboid(24-24)cuboid(37-40)cuboid(59-61)cuboid(73-76)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (4)
cuboid(24-24)cuboid(37-40)cuboid(59-61)cuboid(73-76)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (7)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/segmentindex/SegmentIndexFileService.scala (4)
SegmentIndexFileService(33-205)lookUpSegmentIndexFileKey(46-51)readSegmentIndex(68-78)topLeftsToDistinctTargetMagBucketPositions(78-85)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/Cuboid.scala (2)
mag(46-46)Cuboid(9-60)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/Positions.scala (2)
VoxelPosition(8-47)topLeft(73-79)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (3)
adHocMeshToStl(39-60)combineEncodedChunksToStl(62-71)logMeshingDuration(73-76)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshMappingHelper.scala (1)
segmentIdsForAgglomerateIdIfNeeded(21-68)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/AdHocMeshService.scala (3)
AdHocMeshRequest(30-39)findNeighbors(162-179)requestAdHocMeshViaActor(73-80)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (1)
DataLayer(64-67)
⏰ 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: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (2)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala (1)
681-693: LGTM on switching to parameterless cuboidUsing
request.cuboidhere is correct and consistent with the updated request shape;additionalCoordinatesare forwarded as expected.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (1)
93-114: Confirm target-mag position mapping assumptions.The conversion via
topLeftsToDistinctTargetMagBucketPositions(topLefts, mag)assumestopLeftsare mag1 bucket indices. If any index files store absolute mag1 positions instead, this would underfetch. Consider a short unit/integration test covering both mag=1 and >1 cases.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unreleased_changes/8922.md (1)
1-2: Polish wording; add fallback and backend notes for completenessTighten phrasing and capture key behavior from the PR (fallback for layers without index; backend fullMesh.stl uses the optimization).
-### Added -- If a segmentation layer has a segment index file, it is now used to query ad-hoc meshes. That way, ad-hoc meshes can be correctly fetched also for unconnected segments. +### Added +- When a segmentation layer has a datastore segment index file, it is used for ad‑hoc mesh requests. This enables correct fetching of disconnected components of a segment. +- For layers without a segment index file, ad‑hoc meshes still load but may miss disconnected components. +- The backend `fullMesh.stl` route also leverages the segment index when available.Please confirm whether the preferred terminology in our docs is “datastore segment index (file)” or just “segment index (file)”, and align accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unreleased_changes/8922.md(1 hunks)
⏰ 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: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
|
@MichaelBuessemeyer I now also added the backend part for the fullMesh.stl route. Could you have a quick look? (No need to re-test, I tested the steps in the PR description. If they seem sensible to you, testing can be considered complete.) |
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Backend changes are looking good. I left unrelated improvement.
Moreover, during testing I encountered a bug, where I added the segment index to the wrong dataset first, then removed the index again from the dataset and reloaded it from disk. But then adhoc mesh requests resulted in an error. To reproduce:
- Add the segmentIndex to a wrong dataset (incl datasource-properties.json)
- view the dataset and request adhoc meshes -> fails silently as no vertices for the segment are in the segment index
- remove segment index on disk again and from datasource-properties.json
- reload the dataset from the dashboard with its reloading action
- view the dataset again and request an adhoc mesh. Watch error toasts appearing.
| segmentIndexFileService.readSegmentIndex(segmentIndexFileKey, sId)) | ||
| topLefts: Array[Vec3Int] = topLeftsNested.toArray.flatten | ||
| targetMagPositions = segmentIndexFileService.topLeftsToDistinctTargetMagBucketPositions(topLefts, mag) | ||
| vertexChunksWithNeighbors: List[(Array[Float], List[Int])] <- Fox.serialCombined(targetMagPositions) { |
There was a problem hiding this comment.
Just a thought: We are using serial combined here and got the feedback that adhoc mesh generation is rather slow. So should we maybe parallelize these requests?
There was a problem hiding this comment.
Yes that’s something to keep in mind. But for the moment, I’d like to stick with the policy that answering a single request should not use all the parallelization it can get, especially for compute-heavy stuff like mesh generation. Otherwise, a single request could busy the server and other user’s (simple) requests might not get answered quickly. But that’s certainly something to validate and consider.
There was a problem hiding this comment.
Should we open an issue tagged with discussion to not forget this?
|
@MichaelBuessemeyer Thanks for having a look! Unfortunately I could not reproduce the issue you described. After removing the segment index, reloading the DS in the dashboard and opening it again, I get no error messages on the right-click menu or when loading ad-hoc meshes. Do you happen to remember which request failed, and what the error was? Could an explanation be that the dataset tab was not reinitialized so the frontend code still thought that a segment index existed? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/javascripts/viewer/view/context_menu.tsx (1)
1690-1695: Consider explicitly typing as LayerSourceInfo.Similar to
segment_statistics_modal.tsx, theinfoForRequestUrlobject could benefit from explicitLayerSourceInfotyping and the inclusion of theforceUsingDataStorefield for clarity:- const infoForRequestUrl = { + const infoForRequestUrl: LayerSourceInfo = { dataset, annotation, tracingId, visibleSegmentationLayer, + forceUsingDataStore: undefined, };This improves type safety and makes the intent clearer, though the current implementation is functionally correct if
forceUsingDataStoreis not needed for this use case.frontend/javascripts/admin/rest_api.ts (1)
859-861: Rejected promises cached forever (known issue).As flagged in a previous review,
_.memoizewill cache rejected promises indefinitely. After one transient network failure, all future calls for that key will replay the same rejection without retrying, breaking segment-index lookups until page reload.The memoization resolver correctly uses all arguments as the cache key, but the rejected promise caching remains an issue. Consider wrapping
hasSegmentIndexInDataStoreto clear the cache entry on rejection:-export const hasSegmentIndexInDataStoreCached = _.memoize(hasSegmentIndexInDataStore, (...args) => - args.join("::"), -); +export const hasSegmentIndexInDataStoreCached = _.memoize( + async (...args) => { + const resolverKey = args.join("::"); + try { + return await hasSegmentIndexInDataStore(...args); + } catch (error) { + hasSegmentIndexInDataStoreCached.cache.delete(resolverKey); + throw error; + } + }, + (...args) => args.join("::"), +);Based on learnings
🧹 Nitpick comments (2)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (1)
38-44: Consider narrowing visibleSegmentationLayer type.The
visibleSegmentationLayerfield acceptsAPISegmentationLayer | APIDataLayer | undefined | null, which is quite broad. Since this is specifically for segmentation layer routing, could this be narrowed toAPISegmentationLayer | undefined | null?However, if there are cases where a generic
APIDataLayeris intentionally passed (e.g., when the layer type is not yet determined), the current type is appropriate.frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (1)
103-108: Consider explicitly constructing LayerSourceInfo.The
storeInfoTypeobject matches theLayerSourceInfotype except for the optionalforceUsingDataStorefield. For clarity and type safety, consider explicitly typing this asLayerSourceInfoand including theforceUsingDataStorefield (even if set toundefined):- const storeInfoType = { + const storeInfoType: LayerSourceInfo = { dataset, annotation, tracingId: visibleSegmentationLayer.tracingId, visibleSegmentationLayer, + forceUsingDataStore: undefined, };This makes the intent clearer and provides better type checking. If
forceUsingDataStoreis intentionally omitted for segment statistics, the current implementation is acceptable but less explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/javascripts/admin/rest_api.ts(5 hunks)frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts(2 hunks)frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(9 hunks)frontend/javascripts/viewer/view/context_menu.tsx(2 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx(3 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view_helper.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/javascripts/viewer/view/context_menu.tsx (1)
frontend/javascripts/admin/rest_api.ts (2)
getSegmentVolumes(873-887)getSegmentBoundingBoxes(889-903)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (2)
frontend/javascripts/types/api_types.ts (3)
APIDataset(243-246)APISegmentationLayer(105-113)APIDataLayer(114-114)frontend/javascripts/viewer/store.ts (1)
StoreAnnotation(220-225)
frontend/javascripts/admin/rest_api.ts (3)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (1)
LayerSourceInfo(38-44)frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/admin/api/token.ts (1)
doWithToken(39-74)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (5)
frontend/javascripts/viewer/store.ts (2)
VolumeTracing(192-213)StoreDataset(566-573)frontend/javascripts/admin/rest_api.ts (2)
hasSegmentIndexInDataStoreCached(859-861)getBucketPositionsForAdHocMesh(1947-1973)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)frontend/javascripts/viewer/model.ts (1)
getVisibleSegmentationLayer(122-136)frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (1)
LayerSourceInfo(38-44)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (1)
frontend/javascripts/admin/rest_api.ts (1)
getSegmentBoundingBoxes(889-903)
🔇 Additional comments (17)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (1)
8-13: LGTM!The new imports for
APIDataLayer,APIDataset,APISegmentationLayer,AdditionalCoordinate, andStoreAnnotationare correctly added and used in the newLayerSourceInfotype definition.Also applies to: 30-30
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view_helper.tsx (1)
6-6: LGTM!The removal of
APISegmentationLayerfrom the imports is consistent with the elimination ofgetVolumeRequestUrland the shift toLayerSourceInfo-based routing. The remaining imports are correctly used in the file.frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (2)
20-20: LGTM!The removal of
getVolumeRequestUrlimport is consistent with the shift toLayerSourceInfo-based routing throughout the PR.
130-137: LGTM!The usage of
storeInfoTypewithgetSegmentVolumesandgetSegmentBoundingBoxesis correct and aligns with the updated API signatures that now acceptLayerSourceInfoas the first parameter.frontend/javascripts/viewer/view/context_menu.tsx (2)
149-149: LGTM!The removal of
getVolumeRequestUrlimport is consistent with the broader refactoring to useLayerSourceInfo-based routing instead of direct URL construction.
1702-1709: LGTM!The usage of
infoForRequestUrlwithgetSegmentVolumesandgetSegmentBoundingBoxescorrectly follows the new API pattern that acceptsLayerSourceInfoas the first parameter.frontend/javascripts/admin/rest_api.ts (5)
863-871: Verify if forceUsingDataStore should affect routing.The
getVolumeRequestUrlfunction routes based onannotationandtracingIdpresence, but doesn't consider theforceUsingDataStorefield fromLayerSourceInfo.If
forceUsingDataStoreis intended to override the default routing logic (e.g., force datastore even when annotation/tracingId exist), the function should check this field:export function getVolumeRequestUrl(layerSourceInfo: LayerSourceInfo) { const { dataset, annotation, visibleSegmentationLayer, tracingId } = layerSourceInfo; - if (annotation == null || tracingId == null) { + if (layerSourceInfo.forceUsingDataStore || annotation == null || tracingId == null) { return `${dataset.dataStore.url}/data/datasets/${dataset.id}/layers/${visibleSegmentationLayer?.name}`; } else { const tracingStoreHost = annotation?.tracingStore.url; return `${tracingStoreHost}/tracings/volume/${tracingId}`; } }However, if
forceUsingDataStoreis only used for ad-hoc mesh logic and not for general volume requests, the current implementation is correct.
873-887: LGTM!The refactoring of
getSegmentVolumesto acceptlayerSourceInfoinstead of a rawrequestUrlcorrectly implements the new routing pattern, deriving the URL viagetVolumeRequestUrl.
889-903: LGTM!The refactoring of
getSegmentBoundingBoxesmirrorsgetSegmentVolumesand correctly implements theLayerSourceInfo-based routing pattern.
1899-1906: LGTM!The refactoring of
computeAdHocMeshto acceptdataSourceInfo: LayerSourceInfocorrectly follows the new routing pattern, deriving the request URL viagetVolumeRequestUrl.
1947-1973: LGTM!The refactoring of
getBucketPositionsForAdHocMeshcorrectly:
- Accepts
dataSourceInfo: LayerSourceInfofor routing- Adds
mappingNameparameter for segment index lookups- Derives the request URL via
getVolumeRequestUrl- Includes
mappingNamein the POST bodyfrontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (6)
4-4: LGTM!The new imports for
hasSegmentIndexInDataStoreCached,LayerSourceInfo,StoreDataset, andVolumeTracingare correctly added and used throughout the refactored mesh loading logic.Also applies to: 45-45, 52-52
298-315: LGTM!The
getUsePositionsFromSegmentIndexhelper function correctly determines whether to use the segment index for position lookups based on:
- For static layers (no volumeTracing): checks if a segment index exists in the datastore
- For volume tracings: checks if the tracing has a segment index, no editable mapping, and a valid tracingId
The implementation aligns with the suggestion from past review comments.
344-375: LGTM!The
dataSourceInfocreation correctly:
- Selects necessary state (dataset, annotation, visibleSegmentationLayer)
- Computes
forceUsingDataStorewith appropriate overrides for mesh loading scenarios- Determines whether to use segment index positions via
getUsePositionsFromSegmentIndex- Constructs a complete
LayerSourceInfoobject with all required fieldsThis implementation properly consolidates the routing logic for ad-hoc mesh requests.
378-386: LGTM!The call to
getChunkPositionsFromSegmentIndexcorrectly passes the newdataSourceInfoandmappingNameparameters, aligning with the refactored function signature.
425-442: LGTM!The refactored
getChunkPositionsFromSegmentIndexfunction correctly:
- Accepts
dataSourceInfo: LayerSourceInfofor routing- Accepts
mappingNamefor segment index lookups- Passes both through to
getBucketPositionsForAdHocMesh
451-545: LGTM!The refactored
maybeLoadMeshChunkfunction correctly:
- Accepts
dataSourceInfo: LayerSourceInfoinstead of a booleanuseDataStoreflag- Uses
dataSourceInfo.forceUsingDataStorefor analytics event mode- Passes
dataSourceInfotocomputeAdHocMeshThis aligns with the broader shift to
LayerSourceInfo-based routing throughout the PR.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/admin/rest_api.ts (1)
862-864: Cache rejects forever on first transient failure.
_.memoizestores rejected promises permanently. After one network error, every future call for that key replays the same rejection and never retries, so segment-index lookups stay broken until the page reloads. Clear the cache entry (or avoid memoizing failures) when the request rejects.Wrap
hasSegmentIndexInDataStorewith a function that removes the cache entry on rejection:-export const hasSegmentIndexInDataStoreCached = _.memoize(hasSegmentIndexInDataStore, (...args) => - args.join("::"), -); +const memoizedHasSegmentIndex = _.memoize(hasSegmentIndexInDataStore, (...args) => args.join("::")); +export async function hasSegmentIndexInDataStoreCached(...args: Parameters<typeof hasSegmentIndexInDataStore>) { + try { + return await memoizedHasSegmentIndex(...args); + } catch (error) { + const key = args.join("::"); + memoizedHasSegmentIndex.cache.delete(key); + throw error; + } +}
🧹 Nitpick comments (1)
frontend/javascripts/admin/rest_api.ts (1)
1940-1966: Inconsistent parameter naming: uselayerSourceInfoinstead ofdataSourceInfo.The parameter
dataSourceInfo(line 1941) should be namedlayerSourceInfofor consistency with other updated functions (computeAdHocMesh,getSegmentVolumes, etc.). The type and usage are correct, but the naming is inconsistent.Apply this diff to align naming:
export function getBucketPositionsForAdHocMesh( - dataSourceInfo: LayerSourceInfo, + layerSourceInfo: LayerSourceInfo, segmentId: number, cubeSize: Vector3, mag: Vector3, additionalCoordinates: AdditionalCoordinate[] | null | undefined, mappingName: string | null | undefined, ): Promise<Vector3[]> { - const requestUrl = getDataOrTracingStoreUrl(dataSourceInfo); + const requestUrl = getDataOrTracingStoreUrl(layerSourceInfo); return doWithToken(async (token) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/javascripts/admin/rest_api.ts(5 hunks)frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts(3 hunks)frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(9 hunks)frontend/javascripts/viewer/view/context_menu.tsx(2 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/javascripts/admin/rest_api.ts (3)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (2)
LayerSourceInfo(33-39)getDataOrTracingStoreUrl(276-285)frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/admin/api/token.ts (1)
doWithToken(39-74)
frontend/javascripts/viewer/view/context_menu.tsx (1)
frontend/javascripts/admin/rest_api.ts (2)
getSegmentVolumes(866-880)getSegmentBoundingBoxes(882-896)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (2)
frontend/javascripts/types/api_types.ts (1)
APIDataset(243-246)frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts (1)
annotation(169-221)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (5)
frontend/javascripts/viewer/store.ts (2)
VolumeTracing(192-213)StoreDataset(566-573)frontend/javascripts/admin/rest_api.ts (2)
hasSegmentIndexInDataStoreCached(862-864)getBucketPositionsForAdHocMesh(1940-1966)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)frontend/javascripts/viewer/model.ts (1)
getVisibleSegmentationLayer(122-136)frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (1)
LayerSourceInfo(33-39)
🔇 Additional comments (14)
frontend/javascripts/viewer/view/context_menu.tsx (4)
149-149: LGTM! Import simplified to remove unused function.The removal of
getVolumeRequestUrlis appropriate since the functionality has been replaced with the inlineinfoForRequestUrlobject construction at lines 1690-1695.
1690-1695: Clear inline construction of request info object.The inline
infoForRequestUrlobject is well-structured and explicitly provides all the necessary context (dataset, annotation, tracingId, layer name) required by the updatedgetSegmentVolumesandgetSegmentBoundingBoxesAPIs. This approach aligns with the broader architectural change to useLayerSourceInfo-based routing.
1702-1702: Correct usage of the new API signature.The
infoForRequestUrlobject is properly passed as the first parameter togetSegmentVolumes, matching the updated API signature that acceptsLayerSourceInfo.
1709-1709: Correct usage of the new API signature.The
infoForRequestUrlobject is properly passed as the first parameter togetSegmentBoundingBoxes, consistent with the updated API signature.frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (2)
33-39: LGTM!The
LayerSourceInfotype is well-structured and captures all necessary routing information for data-store vs tracing-store resolution. Field types are appropriate and nullable/undefined handling is consistent.
276-285: LGTM!The routing logic correctly selects the data store or tracing store URL based on annotation presence, tracingId, and forceUsingDataStore. The function centralizes URL resolution and aligns with the PR objectives to use LayerSourceInfo for data-store routing.
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (5)
1-52: LGTM!The new imports (
hasSegmentIndexInDataStoreCached,LayerSourceInfo,StoreDataset,VolumeTracing) are correctly added and align with the updated saga logic for LayerSourceInfo-driven routing.
298-315: LGTM!The
getUsePositionsFromSegmentIndexhelper correctly determines whether to use segment index positions based on volume tracing presence and data-store availability. The logic handles static layers (checking data-store segment index) and volume tracings (checking tracing-store segment index with editable mapping guard) appropriately.
428-448: LGTM!The
getChunkPositionsFromSegmentIndexsignature correctly acceptslayerSourceInfoandmappingName, and passes them togetBucketPositionsForAdHocMesh. This aligns with the LayerSourceInfo-driven routing pattern.
454-548: LGTM!The
maybeLoadMeshChunksignature correctly acceptslayerSourceInfo(replacing theuseDataStoreboolean), and uses it for analytics and mesh computation. The function properly routes requests viacomputeAdHocMesh(layerSourceInfo, ...).
373-376: Incorrect layer name resolution for static layers.When
tracingIdisnullfor a static layer (no volume tracing),visibleSegmentationLayer?.fallbackLayerisundefined, causingvisibleSegmentationLayerNameto beundefined. This breaks the data-store URL (line 280 in wkstore_adapter.ts). For static layers, uselayer.nameinstead.Apply this diff to fix the issue:
visibleSegmentationLayerName: - visibleSegmentationLayer?.tracingId == null - ? visibleSegmentationLayer?.fallbackLayer - : visibleSegmentationLayer?.name, + volumeTracing == null + ? layer.name + : visibleSegmentationLayer?.tracingId == null + ? visibleSegmentationLayer?.fallbackLayer + : visibleSegmentationLayer?.name,Likely an incorrect or invalid review comment.
frontend/javascripts/admin/rest_api.ts (3)
83-86: LGTM!The imports (
LayerSourceInfo,getDataOrTracingStoreUrl) are correctly added and align with the updated API functions that use LayerSourceInfo for routing.
866-896: LGTM!
getSegmentVolumesandgetSegmentBoundingBoxesnow correctly acceptlayerSourceInfoand derive request URLs viagetDataOrTracingStoreUrl(layerSourceInfo). The payload structure remains intact with the required fields.
1892-1938: LGTM!
computeAdHocMeshcorrectly acceptslayerSourceInfoand derives the request URL viagetDataOrTracingStoreUrl(layerSourceInfo). The payload structure is intact with all required mesh request fields.
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/admin/rest_api.ts (1)
862-864: Rejected promises are cached forever.
_.memoizewill cache the rejected promise on the first failure. All subsequent calls with the same arguments will return that cached rejection without retrying, breaking segment-index lookups until the page reloads. Consider clearing the cache entry on rejection or only caching successful results.Apply this pattern to handle failures:
+function hasSegmentIndexWithCacheInvalidationOnError( + dataStoreUrl: string, + datasetId: string, + dataLayerName: string, +) { + const cacheKey = [dataStoreUrl, datasetId, dataLayerName].join("::"); + return hasSegmentIndexInDataStore(dataStoreUrl, datasetId, dataLayerName).catch((error) => { + // Remove from cache on failure to allow retry + hasSegmentIndexInDataStoreCached.cache.delete(cacheKey); + throw error; + }); +} + -export const hasSegmentIndexInDataStoreCached = _.memoize(hasSegmentIndexInDataStore, (...args) => - args.join("::"), -); +export const hasSegmentIndexInDataStoreCached = _.memoize( + hasSegmentIndexWithCacheInvalidationOnError, + (...args) => args.join("::"), +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/rest_api.ts(5 hunks)frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/javascripts/admin/rest_api.ts (2)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (2)
LayerSourceInfo(33-39)getDataOrTracingStoreUrl(276-285)frontend/javascripts/admin/api/token.ts (1)
doWithToken(39-74)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (5)
frontend/javascripts/viewer/store.ts (2)
VolumeTracing(192-213)StoreDataset(566-573)frontend/javascripts/admin/rest_api.ts (2)
hasSegmentIndexInDataStoreCached(862-864)getBucketPositionsForAdHocMesh(1937-1963)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)frontend/javascripts/viewer/model.ts (1)
getVisibleSegmentationLayer(122-136)frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (1)
LayerSourceInfo(33-39)
⏰ 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). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (12)
frontend/javascripts/admin/rest_api.ts (5)
83-86: LGTM: Import additions support the refactor.The new imports
LayerSourceInfoandgetDataOrTracingStoreUrlcentralize URL routing logic, aligning with the architectural goal of moving data-store/tracing-store decision-making away from callers.
866-880: LGTM: Clean refactor to use LayerSourceInfo.The function now derives the request URL internally via
getDataOrTracingStoreUrl(layerSourceInfo), removing the burden from callers and centralizing the data-store/tracing-store routing decision.
882-896: LGTM: Consistent refactor pattern.Same clean refactor as
getSegmentVolumes, centralizing URL derivation throughLayerSourceInfo.
1889-1935: LGTM: Refactor aligns with architectural goal.The function now uses
LayerSourceInfoto derive the request URL, consistent with the broader refactoring pattern.
1937-1963: LGTM: Adds mappingName support for segment index queries.The refactor centralizes URL derivation and adds
mappingNameto the request payload, enabling the backend to apply mappings when retrieving segment index positions. This aligns with the PR objectives.frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (7)
4-4: LGTM: Import additions support the refactor.The new imports enable the saga to use
LayerSourceInfo-based routing and check for segment index availability.Also applies to: 45-45, 52-52
298-315: LGTM: Helper function simplifies segment index logic.The function cleanly separates the logic for determining segment index availability between static layers (checked via datastore) and volume tracings (checked via tracing properties).
362-368: LGTM: Generator function called correctly.The call to
getUsePositionsFromSegmentIndexcorrectly usesyield*, addressing the previously flagged issue. The saga now properly awaits the boolean result.
382-390: LGTM: Refactored call uses LayerSourceInfo.The call to
getChunkPositionsFromSegmentIndexnow passeslayerSourceInfoandmappingName, consistent with the architectural refactor and enabling backend mapping support.
429-446: LGTM: Function signature updated consistently.The function signature and call to
getBucketPositionsForAdHocMeshare consistent with the refactored API inrest_api.ts.
464-464: LGTM: maybeLoadMeshChunk refactored consistently.The function signature and internal usage of
layerSourceInfoare consistent with the broader refactoring effort. Analytics event correctly referenceslayerSourceInfo.forceUsingDataStore.Also applies to: 488-488, 505-505
360-361: LGTM: Improved comment clarity.The updated comment more accurately describes when segment index positions can be used, addressing previous feedback about clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/admin/rest_api.ts (1)
862-864: Cached rejections cause permanent failures.
_.memoizecaches rejected promises indefinitely. After a single transient network failure, all subsequent calls with the same arguments will replay the cached rejection without retrying, effectively breaking segment-index lookups until the page reloads.Wrap the memoized function to clear cache entries on failure:
-export const hasSegmentIndexInDataStoreCached = _.memoize(hasSegmentIndexInDataStore, (...args) => - args.join("::"), -); +const _hasSegmentIndexMemoized = _.memoize(hasSegmentIndexInDataStore, (...args) => + args.join("::"), +); + +export function hasSegmentIndexInDataStoreCached( + dataStoreUrl: string, + datasetId: string, + dataLayerName: string, +) { + const cacheKey = [dataStoreUrl, datasetId, dataLayerName].join("::"); + return _hasSegmentIndexMemoized(dataStoreUrl, datasetId, dataLayerName).catch((err) => { + _hasSegmentIndexMemoized.cache.delete(cacheKey); + throw err; + }); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/admin/rest_api.ts(5 hunks)frontend/javascripts/viewer/model/bucket_data_handling/wkstore_helper.ts(1 hunks)frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_helper.ts (2)
frontend/javascripts/types/api_types.ts (1)
APIDataset(243-246)frontend/javascripts/viewer/store.ts (1)
StoreAnnotation(220-225)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (6)
frontend/javascripts/viewer/store.ts (2)
VolumeTracing(192-213)StoreDataset(566-573)frontend/javascripts/admin/rest_api.ts (2)
hasSegmentIndexInDataStoreCached(862-864)getBucketPositionsForAdHocMesh(1937-1963)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)frontend/javascripts/viewer/model.ts (1)
getVisibleSegmentationLayer(122-136)frontend/javascripts/viewer/model/bucket_data_handling/wkstore_helper.ts (1)
LayerSourceInfo(4-10)frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)
frontend/javascripts/admin/rest_api.ts (3)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_helper.ts (2)
LayerSourceInfo(4-10)getDataOrTracingStoreUrl(12-21)frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/admin/api/token.ts (1)
doWithToken(39-74)
⏰ 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: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (11)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_helper.ts (1)
4-10: LGTM! Clean type definition for centralized routing.The
LayerSourceInfotype appropriately captures all necessary context for determining data vs. tracing store URLs. The union ofnull | undefinedon optional fields provides flexibility for different call sites.frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (5)
298-315: LGTM! Clean helper for segment index decision.The
getUsePositionsFromSegmentIndexfunction correctly encapsulates the logic for determining whether to use the segment index: it checks for static layers via datastore or volume tracings with appropriate flags.
362-368: LGTM! Generator properly invoked.The call to
getUsePositionsFromSegmentIndexcorrectly usesyield*to await the saga result, addressing the previous critical issue.
370-379: VerifyvisibleSegmentationLayerNameassignment logic.The ternary at lines 374-377 assigns
fallbackLayerwhentracingId != null, butfallbackLayercan beundefined(perVolumeTracingtype), resulting inundefinedbeing passed to the datastore URL builder.Based on past review discussions, it seems the intent is: when there's a volume tracing with a fallback layer, use the fallback; otherwise use the layer name. Consider using nullish coalescing to guard against
undefined:visibleSegmentationLayerName: visibleSegmentationLayer?.fallbackLayer ?? visibleSegmentationLayer?.name,This ensures a valid layer name is always provided. Please confirm whether the current ternary logic matches the expected behavior for your use case.
429-449: LGTM! Signature refactored to useLayerSourceInfo.The function now correctly accepts
LayerSourceInfoandmappingName, aligning with the centralized routing pattern. The propagation togetBucketPositionsForAdHocMeshis appropriate.
455-549: LGTM! Refactored to propagateLayerSourceInfo.The function signature change from boolean
useDataStoretoLayerSourceInfois consistent with the broader refactoring. All downstream usages (analytics,computeAdHocMesh) correctly referencelayerSourceInfo.frontend/javascripts/admin/rest_api.ts (5)
83-86: LGTM! Imports align with refactoring.The imports of
LayerSourceInfoandgetDataOrTracingStoreUrlsupport the centralized URL routing pattern introduced in this PR.
866-880: LGTM! Refactored to useLayerSourceInfo.The function signature now correctly accepts
LayerSourceInfoand derives the request URL viagetDataOrTracingStoreUrl. The inclusion ofmappingNamein the POST data is appropriate for segment statistics.
882-896: LGTM! Consistent refactoring.The
getSegmentBoundingBoxesfunction follows the same pattern asgetSegmentVolumes, correctly usingLayerSourceInfofor URL derivation and includingmappingNamein the request payload.
1889-1935: LGTM! Ad-hoc mesh computation refactored.The
computeAdHocMeshfunction now acceptsLayerSourceInfoand derives the request URL appropriately. The refactoring maintains the existing mesh computation logic while enabling consistent routing.
1937-1963: LGTM! Segment index positions refactored.The function signature now accepts
LayerSourceInfoandmappingName, enabling the backend to use segment index files for ad-hoc mesh loading. The inclusion ofmappingNamein the POST data is correct.
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_helper.ts
Show resolved
Hide resolved
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_helper.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (1)
103-108: Use LayerSourceInfo naming and correct tracingId sourceRename storeInfoType to layerSourceInfo for consistency, and prefer the component’s tracingId prop over visibleSegmentationLayer.tracingId to avoid mismatches between view state and layer metadata.
Apply:
- const storeInfoType = { + const layerSourceInfo = { dataset, annotation, - tracingId: visibleSegmentationLayer.tracingId, + tracingId, // prefer prop; may be undefined for static layers segmentationLayerName: visibleSegmentationLayer.name, };And update calls:
- getSegmentVolumes( - storeInfoType, + getSegmentVolumes( + layerSourceInfo, layersFinestMag, segments.map((segment) => segment.id), additionalCoordinates, maybeGetMappingName(), ),- getSegmentBoundingBoxes( - storeInfoType, + getSegmentBoundingBoxes( + layerSourceInfo, layersFinestMag, segments.map((segment) => segment.id), additionalCoordinates, maybeGetMappingName(), ),Also applies to: 130-135, 137-143
frontend/javascripts/viewer/view/context_menu.tsx (3)
517-523: Proofreading label should display unmapped IDIn proofreading, segmentIdLabel should show the unmapped segment ID (maybeUnmappedSegmentId), not the mapped/mesh ID. This aids users per prior UX decisions.
Consider aligning the label in getMeshItems:
Example adjustment (outside this hunk):
// Prefer unmapped ID in proofreading const segmentIdLabel = isProofreadingActive && maybeUnmappedSegmentId != null ? `within Segment ${maybeUnmappedSegmentId}` : clickedMeshId;Based on learnings
1172-1172: Remove empty onOk handlerNo-op onOk adds noise and may confuse readers.
- onOk() { }, + // no onOk needed
1387-1390: Minor: fix menu item key typoKey “compute-mesh-adhc” likely intended “adhoc”. Cosmetic but keeps keys consistent.
- key: "compute-mesh-adhc", + key: "compute-mesh-adhoc",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/viewer/view/context_menu.tsx(13 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/view/context_menu.tsx
🧬 Code graph analysis (2)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (1)
frontend/javascripts/admin/rest_api.ts (1)
getSegmentBoundingBoxes(882-896)
frontend/javascripts/viewer/view/context_menu.tsx (9)
frontend/javascripts/viewer/model/actions/proofread_actions.ts (5)
minCutPartitionsAction(107-110)proofreadMergeAction(45-55)minCutAgglomerateWithPositionAction(64-74)cutAgglomerateFromNeighborsAction(76-88)minCutAgglomerateAction(57-62)frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
setActiveCellAction(206-218)frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (6)
mergeTreesAction(465-470)deleteEdgeAction(228-240)deleteBranchpointByIdAction(295-300)createBranchPointAction(278-288)setTreeVisibilityAction(412-417)createTreeAction(307-317)frontend/javascripts/viewer/controller/combinations/skeleton_handlers.ts (1)
handleCreateNodeFromGlobalPosition(258-288)frontend/javascripts/viewer/model/accessors/skeletontracing_accessor.ts (1)
areGeometriesTransformed(240-246)frontend/javascripts/viewer/controller/combinations/segmentation_handlers.ts (2)
loadAgglomerateSkeletonAtPosition(25-34)loadSynapsesOfAgglomerateAtPosition(57-77)frontend/javascripts/viewer/model/actions/dataset_actions.ts (1)
ensureLayerMappingsAreLoadedAction(48-52)frontend/javascripts/admin/rest_api.ts (2)
getSegmentVolumes(866-880)getSegmentBoundingBoxes(882-896)
⏰ 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). (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (11)
frontend/javascripts/viewer/view/context_menu.tsx (11)
438-443: Good addition: partition split action surfacedExposing “Split partitions” when both partitions have entries is correct and improves discoverability.
527-542: Merge action wiring looks correctProper guards and parameter order for proofreadMergeAction; disables self-merge; tooltip wiring OK.
544-562: Min-cut at position action looks correctGuards prevent invalid states; action/minCut parameters are consistent.
564-581: Neighbor split action OKReasonable disable conditions; action parameters match the API.
1182-1208: Skeleton actions gating is soundGood guards for geometry transforms, mappings, and tool state; UX tooltips are clear.
1240-1250: Multi-split options integration LGTMCorrectly reuses getMultiCutToolOptions with unmapped/mapped IDs and labels.
1394-1428: Segment activation/visibility entries look goodCorrectly guards against segment 0 and disabled tool states; actions/labels consistent.
1691-1714: LayerSourceInfo usage for stats fetches LGTMBuilding layerSourceInfo and routing through getSegmentVolumes/getSegmentBoundingBoxes aligns with the new API.
1770-1775: Distance-to-selection calculation OKScaled/voxel distances computed and formatted correctly.
1938-1948: Menu item construction spread is correctProps spread ordering preserves overrides; no issues.
589-603: Use segmentId 0 to clear active selection
Replace the dispatch in the “deactivate-segment” menu (frontend/javascripts/viewer/view/context_menu.tsx) to pass 0 instead of clickedMeshId:- onClick: () => - Store.dispatch(setActiveCellAction(clickedMeshId, undefined, undefined, undefined)), + onClick: () => + Store.dispatch(setActiveCellAction(0, undefined, undefined, undefined)),0 is reserved for “no annotation” and tests confirm it clears the active cell without creating a cell 0.
Likely an incorrect or invalid review comment.
|
Ok I just did some double checking :D I think the bug occurs due to the segment index you provided being not complete and thus the answer of the server is If you think that's correct. Let's merge this :D Else, this might need some more investigation |
|
Yes, looks like the segment index file doesn’t cover the full id range. I made a new one now and with that I don’t get those errors any more. |
- Adds surface area segment stat, based on mesh triangle surfaces - Fixes fullMesh loading with ad-hoc meshes, also used in render_animation worker jobs ### URL of deployed dev instance (used for testing): - https://meshsurfacearea.webknossos.xyz ### Steps to test: - For dataset with segment index file, and also for volume annotation: - Context menu should now have explicitly triggered loading of segment statistics - Surface area should be included ### TODOs: - [x] Fix Infinity/NaN - [x] Validate math/output value against meshlab - [x] move to segment statistics route - [x] also allow for adhoc/volume/proofreading? - [x] Frontend - [x] Add surface area segment statistic - [x] Move segment statistic out of toplevel rightclick menu to save computation - [x] Select correct combination of meshing parameters for the new route - [x] Wait for #8922 to enable static layer ad-hoc without seedPosition - [x] Re-Test proofreading ad-hoc, do we need to set editableMappingTracingId or is the layer doing this for us already? ### Issues: - fixes #8610 ------ - [x] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`) - [x] Removed dev-only changes like prints and application.conf edits - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) - [x] Needs datastore update after deployment --------- Co-authored-by: Philipp Otto <[email protected]> Co-authored-by: Michael Büßemeyer <[email protected]>
URL of deployed dev instance (used for testing):
Steps to test:
3629, 3489, 1024(Segment 41)curl 'http://localhost:9000/data/datasets/68da42fbd90000f3132f9921/layers/segmentation/meshes/fullMesh.stl?token=secretSampleUserToken' -X POST -H 'Accept-Encoding: gzip, deflate, br' -H 'content-type: application/json' --data-raw '{"segmentId": 41, "mag": [8, 8, 2]}' --output ~/out.stl(insert dataset id from your local wk instance), inspect result, e.g. in meshlab. Should also show both pieces.Issues:
some context: https://scm.slack.com/archives/C5AKLAV0B/p1757940808856039
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)