Don't label outside of layer's bbox #8602
Conversation
…sables corresponding check in shader)
📝 WalkthroughWalkthroughThis change introduces containment-aware logic to prevent labeling voxels outside the dataset's bounding box. Bucket creation and voxel labeling routines are updated to compute and enforce containment, restricting voxel writes to regions within the bounding box. Related tests and type definitions are updated, and a changelog entry documents the fix. Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…tely, but it got too complicated)
philippotto
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: 8
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/shaders/coords.glsl.ts (1)
71-72:⚠️ Potential issueRemove temporary debug code
This code forces
isOutsideOfBoundingBoxto always returnfalse, which would bypass all bounding box checking. The "todop: remove again" comment confirms this is temporary debugging code that shouldn't be committed.Please remove these lines to restore the proper bounding box checking functionality:
- // todop: remove again - return false;
🧹 Nitpick comments (12)
CHANGELOG.unreleased.md (1)
27-27: Clear and accurate changelog entry.This entry correctly summarizes the fix that prevents voxels outside the layer bounding box from being brushed, matching the PR objectives.
Minor stylistic suggestion: Consider "Voxels outside the layer bounding box" instead of "outside of".
-Voxels outside of the layer bounding box cannot be brushed, anymore. [#8602](https://github.com/scalableminds/webknossos/pull/8602) +Voxels outside the layer bounding box cannot be brushed anymore. [#8602](https://github.com/scalableminds/webknossos/pull/8602)🧰 Tools
🪛 LanguageTool
[style] ~27-~27: This phrase is redundant. Consider using “outside”.
Context: ...bleminds/webknossos/pull/8599) - Voxels outside of the layer bounding box cannot be brushe...(OUTSIDE_OF)
frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts (1)
178-180: Prevent updating segment ID 0This guard clause prevents updates to segment ID 0, which is a critical part of the PR's objective to respect boundary constraints during voxel labeling operations.
Consider adding a short comment explaining why segment ID 0 needs special handling:
if (segmentId === 0) { + // Skip updates for segment ID 0 as it represents background/empty space return segments; }frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts (1)
346-352: Shallow-copy ofBucketAddresscan still leak mutations into the original array
[...labeledBucketZoomedAddress]copies the tuple only one level deep.
If the 5-th element (theAdditionalCoordinate[]) is notnull, the inner
array is shared between the original andnextBucketZoomedAddress. While you
currently only increment the numeric part (nextBucketZoomedAddress[thirdDimensionIndex]++),
future modifications (e.g. pushing an additional coordinate) would silently
mutate the original object and break the readonly guarantee you have just
introduced.-const nextBucketZoomedAddress: Writeable<BucketAddress> = [...labeledBucketZoomedAddress]; +const nextBucketZoomedAddress: Writeable<BucketAddress> = [ + ...labeledBucketZoomedAddress.slice(0, 4), + labeledBucketZoomedAddress[4] != null + ? [...labeledBucketZoomedAddress[4]] // deep-copy the additional-coordinate list + : null, +] as Writeable<BucketAddress>;This keeps the immutable promise of
BucketAddressintact and prevents
accidental side effects.frontend/javascripts/oxalis/constants.ts (1)
52-56: Spelling:Vector*Indicies→Vector*IndicesTiny nit – “indices” is the correct plural of “index”.
-export const Vector4Indicies = [0, 1, 2, 3] as const; +export const Vector4Indices = [0, 1, 2, 3] as const;(The same applies to the other constant arrays for consistency.)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (2)
125-131: Document the newcontainmentconstructor parameterThe extra parameter is non-obvious for callers skimming the signature.
Adding a short JSDoc snippet (expected coordinate space, inclusive/exclusive
semantics) will spare future confusion when creating buckets in other
modules/tests.
528-546: Iteration bounds are recomputed for every voxel-write
Dimensions.planeForThirdDimension()andDimensions.getIndices()are called
for everyapplyVoxelMapinvocation. For heavy labeling operations this can
be a hot path. Caching the(u,v,w)index triple once outside the per-bucket
loop (or even on the bucket instance) would avoid repeated allocations and
string → enum conversions.frontend/javascripts/test/model/volumetracing/volume_annotation_sampling.spec.ts (2)
67-68: Avoid hard-coding dataset bounds in tests
cubeBoundingBoxis currently defined with magic numbers[1, 2, 3] … [1023, …].
IfConstants.BUCKET_WIDTHor the dataset size changes, the tests may silently become invalid.
Consider deriving the bounds fromConstants.BUCKET_WIDTHorcube.getSize()instead, e.g.-const cubeBoundingBox = new BoundingBox({ min: [1, 2, 3], max: [1023, 1024, 1025] }); +const max = Constants.BUCKET_WIDTH * 32; // or cube.getSize() +const cubeBoundingBox = new BoundingBox({ min: [1, 2, 3], max: [max, max + 1, max + 2] });
667-678: Reuse BoundingBox helpers instead of manual range checksThe expected–data loop manually checks whether each
(firstDim, secondDim)lies inside the bounding box:if (firstDim < cubeBoundingBox.min[0] || …) { return; }This duplicates logic that already exists in
BoundingBox.containsPoint.
Using the helper clarifies intent and prevents off-by-one mistakes:- if ( - firstDim < cubeBoundingBox.min[0] || - secondDim < cubeBoundingBox.min[1] || - firstDim >= cubeBoundingBox.max[0] || - secondDim >= cubeBoundingBox.max[1] - ) { - return; - } + if (!cubeBoundingBox.containsPoint([firstDim, secondDim, Z])) return;frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts (1)
88-103: Explicit “empty” intersection would improve downstream checks
intersectedWithFastcollapses non-overlapping boxes to a zero-volume
box (min == max). Call-sites that only test equality might
misinterpret this as a “real” box. Consider returningnull(or an
isEmptyflag) to make empty intersections explicit and avoid accidental
processing of degenerate boxes.frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (3)
73-92: Local-coordinate helpers look good but can be micro-simplifiedThe helper functions achieve the desired mapping from global → local bucket coordinates, handling the “32 instead of 0” edge case. Two small improvements:
- The
zeroToBucketWidthlambda is recreated for every import of this file. Consider an inline ternary to avoid an extra closure entirely.mod(Math.floor(x / mag[i]), Constants.BUCKET_WIDTH)is executed three times per call. Whenmagis guaranteed to be positive, using plain remainder (%) is sufficient and ~20 % faster.These are micro-optimisations—feel free to ignore if readability is preferred.
351-358: Address collision guard: good idea, but_.isEqualis overkillUsing a defensive check to avoid collisions when linearising addresses
is 👍. However,_.isEqualincurs an extra allocation and deep-compare.A manual compare of four integers is ~8× faster and clearer:
- if (bucket != null && _.isEqual(address.slice(0, 4), bucket.zoomedAddress.slice(0, 4))) { + if ( + bucket != null && + address[0] === bucket.zoomedAddress[0] && + address[1] === bucket.zoomedAddress[1] && + address[2] === bucket.zoomedAddress[2] && + address[3] === bucket.zoomedAddress[3] + ) {
530-534: Deprecation notice is helpful—consider adding a runtime warningThe explanatory comment is great. To make accidental production usage
even less likely, emit aconsole.warnifNODE_ENV !== "production"
when this method is invoked.- // This function is only provided for testing purposes … + if (process.env.NODE_ENV !== "production") { + console.warn( + "_labelVoxelInResolution_DEPRECATED is for tests only and will be removed soon.", + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/libs/utils.ts(1 hunks)frontend/javascripts/oxalis/api/api_latest.ts(12 hunks)frontend/javascripts/oxalis/api/wk_dev.ts(2 hunks)frontend/javascripts/oxalis/constants.ts(2 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts(3 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts(8 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts(7 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/pullqueue.ts(3 hunks)frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts(1 hunks)frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts(3 hunks)frontend/javascripts/oxalis/shaders/coords.glsl.ts(1 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(1 hunks)frontend/javascripts/test/model/binary/cube.spec.ts(2 hunks)frontend/javascripts/test/model/binary/layers/wkstore_adapter.spec.ts(1 hunks)frontend/javascripts/test/model/binary/pullqueue.spec.ts(1 hunks)frontend/javascripts/test/model/binary/temporal_bucket_manager.spec.ts(4 hunks)frontend/javascripts/test/model/texture_bucket_manager.spec.ts(1 hunks)frontend/javascripts/test/model/volumetracing/volume_annotation_sampling.spec.ts(6 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
frontend/javascripts/test/model/binary/pullqueue.spec.ts (1)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (1)
DataBucket(93-833)
frontend/javascripts/test/model/binary/temporal_bucket_manager.spec.ts (1)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (1)
DataBucket(93-833)
frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts (2)
frontend/javascripts/libs/utils.ts (1)
Writeable(18-18)frontend/javascripts/oxalis/constants.ts (1)
BucketAddress(22-24)
frontend/javascripts/test/model/binary/cube.spec.ts (2)
frontend/javascripts/oxalis/constants.ts (1)
Vector3(13-13)frontend/javascripts/oxalis/model/helpers/mag_info.ts (1)
MagInfo(11-225)
frontend/javascripts/oxalis/api/wk_dev.ts (1)
frontend/javascripts/oxalis/singletons.ts (1)
Model(7-7)
frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts (2)
frontend/javascripts/oxalis/constants.ts (3)
Vector3(13-13)BoundingBoxType(41-44)Vector4(14-14)frontend/javascripts/libs/mjs.ts (1)
V3(398-398)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (1)
frontend/javascripts/oxalis/constants.ts (1)
Vector3(13-13)
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (3)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (3)
DataBucket(93-833)Containment(80-91)Bucket(70-70)frontend/javascripts/oxalis/constants.ts (2)
Vector3(13-13)BucketAddress(22-24)frontend/javascripts/libs/utils.ts (1)
mod(24-26)
frontend/javascripts/oxalis/constants.ts (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (2)
AdditionalCoordinate(90-95)AdditionalCoordinate(97-112)
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~27-~27: This phrase is redundant. Consider using “outside”.
Context: ...bleminds/webknossos/pull/8599) - Voxels outside of the layer bounding box cannot be brushe...
(OUTSIDE_OF)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (19)
frontend/javascripts/libs/utils.ts (1)
18-18: Great utility type addition!This is a well-implemented TypeScript utility type that removes the
readonlymodifier from properties. It will be useful for type-safe operations that need to temporarily modify otherwise readonly types.frontend/javascripts/oxalis/model/bucket_data_handling/pullqueue.ts (3)
34-34: LGTM: Added property for tracking destruction state.This boolean flag will help manage the lifecycle of the PullQueue instance.
109-111: Good error handling improvement.Early returning when the queue is destroyed prevents unnecessary error processing after the instance has been terminated, avoiding potential side effects.
205-209: Well-designed destroy method.This method provides a clean way to terminate the queue's operations by:
- Setting the destroyed flag to prevent further operations
- Clearing the queue (while preserving highest priority items)
- Aborting any ongoing requests
This is a good pattern for resource cleanup.
frontend/javascripts/test/model/texture_bucket_manager.spec.ts (1)
38-38: LGTM: Updated DataBucket constructor with containment typeThe addition of
{ type: "full" }correctly updates the test to align with the newDataBucketconstructor signature that supports containment types, which is essential for the bounding box containment feature being implemented in this PR.frontend/javascripts/test/helpers/apiHelpers.ts (1)
193-193: Code cleanup: Using consolidated destroy() methodGood refactoring to replace separate
clear()andabortRequests()calls with a singledestroy()method, improving code maintainability.frontend/javascripts/oxalis/api/wk_dev.ts (2)
7-7: LGTM: Updated import to include ModelAppropriate addition of Model to the import from singletons to support the new getter.
62-65: Added model getter for debugging supportThis addition provides a convenient way to access the Model singleton for debugging purposes, complementing the existing store getter. Since this class is explicitly for debugging and not an official API, this is a reasonable addition.
frontend/javascripts/test/model/binary/pullqueue.spec.ts (1)
86-87: Test code correctly updated for new DataBucket constructor signatureThe test now properly includes the new
containmentparameter with{ type: "full" }when instantiating DataBucket objects, which aligns with the PR's goal of making buckets respect bounding box containment.frontend/javascripts/test/model/binary/temporal_bucket_manager.spec.ts (1)
56-57: Test code correctly updated for new DataBucket constructor signatureAll DataBucket instantiations have been properly updated to include the new
containmentparameter with{ type: "full" }, conforming to the updated constructor signature. These changes maintain test functionality while supporting the PR's goal of bounding box containment.Also applies to: 63-64, 73-74, 84-85, 96-97
frontend/javascripts/test/model/binary/layers/wkstore_adapter.spec.ts (1)
253-259: Test code correctly updated for new DataBucket constructor signatureBoth DataBucket instantiations have been properly updated to include the new
containmentparameter with{ type: "full" }, conforming to the updated constructor signature. These changes maintain test functionality while supporting the PR's goal of bounding box containment.Also applies to: 264-270
frontend/javascripts/test/model/binary/cube.spec.ts (1)
88-92: Update assertion helpers to reflect removed “bounds-check” flagThe removed
trueargument meansgetBucketalways enforces the
bbox-containment internally. Make sure helper assertions (e.g. tests that
expect an out-of-bbox bucket to benull) are still valid under the new
behaviour; otherwise the tests could turn into false positives.frontend/javascripts/oxalis/constants.ts (1)
23-24: 👍 Read-onlyBucketAddressdramatically improves type-safetyMaking the tuple immutable forces callers to be explicit (via
Writeable)
when they really need to mutate the address. Nice improvement!frontend/javascripts/oxalis/api/api_latest.ts (2)
2215-2233: Potential XY-indexing mix-up when setting bits inlabelMap
labelMap[localA * BUCKET_WIDTH + localB] = 1assumes
row-major(x, y)layout, butapplyVoxelMapinternally treats its map
as(y, x)(column-major). If that is the case, labels are rotated by
90°. Please double-check the expected ordering and swap the indices if
necessary:-labelMap[localA * Constants.BUCKET_WIDTH + localB] = 1; +labelMap[localB * Constants.BUCKET_WIDTH + localA] = 1; // if column-major
2248-2263:numberOfSliceshard-coded to 1
applyVoxelMapreceivesnumberOfSlices = 1, yet the function is
executed inside a loop that already fixes a singlewlayer. If the
voxel map ever spans more than one XY slice inside the same bucket,
those additional slices will be silently dropped. Either compute the
actual slice count or document the limitation.frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (3)
241-280: Containment logic is correct; watch for negative coordinatesGreat job replacing the boolean “inside?” check with the more expressive
Containmenttri-state. The fast-path exits cover 99 % of calls.Two things to keep in mind:
BoundingBox.fromBucketAddressFastandmakeLocal*assume the global
min/max can be divided bymagwithout rounding surprises. If your
dataset ever contains negative coordinates,Math.floorbehaves
differently from integer division. (Yourmodhelper keeps the result
non-negative, which is good.)- This method allocates three
BoundingBoxinstances per call
(bucketBBox,intersectionBBox, the result ofintersectedWithFast).
If profiling shows this is hot, consider an in-place intersection to
reuse objects.Overall: ✅ functional, ❓ potential perf if called in tight loops.
366-372: Constructor signature change lacks type-safety downstream
createBucketnow forwardscontainmenttoDataBucket, but the
call-sites in the rest of the file haven’t been updated to pass a typed
Containmentobject when directly instantiatingDataBucket
(only this helper does).
Search the repo fornew DataBucket(; any external instantiation must
be adapted, otherwise TypeScript will happily acceptcontainment: any.Recommend adding
type NewDataBucketParams = ConstructorParameters<typeof DataBucket>;and exporting it, to help catching wrong calls during compilation.
231-239:⚠️ Potential issue
getCubeKeyproduces two keys for the same semantics (nullvs[])When
allCoordsisnullyou return"${zoomStep}",
but when it is an empty array you return"${zoomStep}-".Both represent “no additional coordinates” yet create two distinct map
entries, leaking memory and duplicating buckets.- const relevantCoords = allCoords.filter((coord) => this.additionalAxes[coord.name] != null); - return [zoomStep, ...relevantCoords.map((el) => el.value)].join("-"); + const relevantCoords = allCoords.filter( + (coord) => this.additionalAxes[coord.name] != null, + ); + if (relevantCoords.length === 0) { + return `${zoomStep}`; + } + return [zoomStep, ...relevantCoords.map((el) => el.value)].join("-");Likely an incorrect or invalid review comment.
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts (1)
451-451: Formatting alignment only. This change simply realigns the closing comment delimiter to match the indentation of the opening delimiter. No functional impact.
frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (1)
536-565: 🛠️ Refactor suggestionMissing runtime safeguard for buckets outside the layer bbox
While the code handles the "partial" case correctly, there's no explicit check for the
"no"containment type.Add an explicit early-return to avoid accidental writes when a bucket is completely outside the layer's bounding box:
+if (this.containment.type === "no") { + return false; // bucket fully outside bbox – nothing to do +}This ensures voxels will never be modified in buckets that should be excluded entirely.
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (1)
20-21: Duplicate default import from the same module
constantsis already imported as the default export on line 20. Importing the default again under the aliasConstantson line 21 works syntactically, but:
• it adds bytes to every bundle that includes this file, because the bundler deduplicates the module after symbol mangling,
• it can easily confuse future readers ("areconstantsandConstantsdifferent?").Prefer a single import and reuse the same identifier:
-import constants, { MappingStatusEnum } from "oxalis/constants"; -import Constants from "oxalis/constants"; +import constants, { MappingStatusEnum } from "oxalis/constants"; /* if you need an alias, do: +const Constants = constants; +*/
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (1)
549-565: Missing optimization for out-of-bounds slicesWhen
sliceOffsetis completely outside the w-dimension limits for a partially contained bucket, the function still iterates through all U×V positions only to skip them later.Consider adding an early-return optimization to skip processing slices that are entirely outside the allowed W-range:
+if ( + this.containment.type === "partial" && + (sliceOffset < limits.w.min || sliceOffset >= limits.w.max) +) { + return false; +}This would save O(32²) operations per slice, which is especially beneficial for thin bbox slabs.
frontend/javascripts/oxalis/api/api_latest.ts (2)
2190-2192: ExplicitnulladditionalCoordinates are ignoredconst additionalCoordinates = optAdditionalCoordinates === undefined ? state.flycam.additionalCoordinates : optAdditionalCoordinates;Passing
nullexplicitly is indistinguishable from passingundefined; both fall back to the fly-cam coordinates. This prevents a caller from explicitly requesting "no additional coordinates".Consider checking specifically for
undefinedrather than using the nullish coalescing operator:-const additionalCoordinates = - optAdditionalCoordinates === undefined - ? state.flycam.additionalCoordinates - : optAdditionalCoordinates; +const additionalCoordinates = + optAdditionalCoordinates === undefined + ? state.flycam.additionalCoordinates + : optAdditionalCoordinates;
2237-2266: Inefficient per-sliceapplyVoxelMapcallsThe loop runs
applyVoxelMapand the expensiveapplyLabeledVoxelMapToAllMissingMagsonce per Z slice. For large brushes affecting many slices, this scales poorly (O(slices * buckets)).Consider building one
LabeledVoxelsMapfor the entire call and invoking the functions only once to avoid repeated traversal and queueing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/oxalis/api/api_latest.ts(12 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts(3 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts(8 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (1)
frontend/javascripts/oxalis/constants.ts (1)
Vector3(13-13)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (8)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (3)
80-93: Well-designed containment type system for bucket handlingThe new
SomeContainmentandContainmenttypes provide a clean representation of how buckets intersect with the layer's bounding box. The distinction between "no", "full", and "partial" containment with inclusive min and exclusive max bounds is a solid design.
131-131: Added containment parameter to DataBucket constructorThe constructor now accepts a containment parameter to store information about how the bucket intersects with the layer's bounding box, which will be used when modifying voxels.
530-547: Proper handling of partial containment limitsThe implementation correctly sets up dimension-specific limits for u, v, and w coordinates when dealing with partial containment. This ensures that operations respect the layer's bounding box.
frontend/javascripts/oxalis/api/api_latest.ts (1)
2182-2280: Significant improvement in voxel labeling approachThe rewrite of
labelVoxelsshifts from per-voxel labeling to a much more efficient batch approach. The code now:
- Groups voxels by their third dimension coordinate
- Constructs labeled voxel maps for each bucket
- Applies those maps using proper containment-aware functions
This implementation properly respects layer bounding boxes and should be more performant.
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (4)
77-96: Well-designed helper functions for containment calculationsThe constants and helper functions for creating local min/max coordinates are well-designed and help encapsulate the logic needed for partial containment. The
makeLocalMinandmakeLocalMaxfunctions handle the tricky modulo calculations cleanly.
245-284: Enhanced containment checking replaces simple boolean checkThe new
checkContainmentmethod provides much more detailed information than a boolean check:
- Identifies if a bucket is completely outside the bounding box
- Determines if a bucket is fully contained in the bounding box
- For partial containment, calculates the exact intersection coordinates
This enables more precise voxel operations that respect the layer's boundaries.
334-346: Simplified bucket creation with containment awarenessThe
getOrCreateBucketmethod now uses the detailed containment information to decide whether to create a bucket, improving upon the previous approach. This integrates well with the new containment-aware functionality.
527-562: Clear documentation for deprecated methodThe comment for
_labelVoxelInResolution_DEPRECATEDnow clarifies that it should only be used for testing purposes and suggests using the more efficientLabeledVoxelsMapapproach instead. This helps guide developers toward the correct pattern.
There was a problem hiding this comment.
Nice, thanks for this PR and also improving performance 🎉 (at least that how I understand parts of this PR).
Codewise I did not really find anything. I just left a few suggestions.
Testing:
Everything worked as expected except for one small detail: During testing with restricted mags (4-4-1) the area where drawing is allowed is a little off by -1 in x and y direction (did not test z direction). Please see the image: The gray DS border is not clearly where the turquoise border (my brushing) is. When no mag restriction was in place, everything worked fine :D
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/bucket_data_handling/pullqueue.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (1)
21-22: Duplicate default import from the same module
constantsis already imported as the default export on line 21. Importing the default again under the aliasConstantson line 22 is redundant.-import constants, { MappingStatusEnum } from "oxalis/constants"; -import Constants from "oxalis/constants"; +import constants, { MappingStatusEnum } from "oxalis/constants"; +const Constants = constants;frontend/javascripts/oxalis/api/api_latest.ts (3)
2213-2237: Label application is performed per sliceThe labelVoxels implementation processes voxels grouped by their third dimension (z), creating and applying a separate voxel map for each slice. This approach works well for small brushes but could be inefficient for large brushes affecting many slices.
Consider building one complete
LabeledVoxelsMapfor the entire operation and invokingapplyVoxelMapandapplyLabeledVoxelMapToAllMissingMagsjust once to avoid repeated traversal and queueing, especially when labeling large volumes with many slices:-for (const group of Object.values(groupedByW)) { - const w = group[0][thirdDim]; - const currentLabeledVoxelMap: LabeledVoxelsMap = new Map(); - - for (const pos of group) { - // Processing for each position in the group - } - - // Apply for this slice... - applyVoxelMap(...); - applyLabeledVoxelMapToAllMissingMags(...); -} +// Build one complete map +const completeLabeledVoxelMap: LabeledVoxelsMap = new Map(); + +for (const group of Object.values(groupedByW)) { + const w = group[0][thirdDim]; + + for (const pos of group) { + // Add to the complete map... + } +} + +// Apply once for all slices +applyVoxelMap(completeLabeledVoxelMap, ...); +applyLabeledVoxelMapToAllMissingMags(completeLabeledVoxelMap, ...);
2246-2246: Usewto avoid conversionconst w = parseInt(thirdDim);This conversion is unnecessary since
group[0][thirdDim]would already give you the same value.-const w = parseInt(thirdDim); +const w = group[0][thirdDim];
2190-2192: 🛠️ Refactor suggestionHandling of optional additionalCoordinates could be improved
The current implementation:
const additionalCoordinates = optAdditionalCoordinates === undefined ? state.flycam.additionalCoordinates : optAdditionalCoordinates;should distinguish between
undefinedandnullvalues explicitly, as passingnullintentionally is indistinguishable from passingundefined.-const additionalCoordinates = - optAdditionalCoordinates === undefined - ? state.flycam.additionalCoordinates - : optAdditionalCoordinates; +const additionalCoordinates = + optAdditionalCoordinates === undefined + ? state.flycam.additionalCoordinates + : optAdditionalCoordinates;
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/api/api_latest.ts (1)
2270-2270: Consider renaming 'sliceOffset' for clarityThe variable name
numberOfSlices(used as the 5th argument toapplyVoxelMap) could be renamed to something more descriptive likesliceOffsetorsliceCountto better reflect its purpose in the function.-const numberOfSlices = 1; +const sliceOffset = 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/libs/utils.ts(1 hunks)frontend/javascripts/oxalis/api/api_latest.ts(12 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts(7 hunks)frontend/javascripts/types/globals.d.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/javascripts/libs/utils.ts
- frontend/javascripts/types/globals.d.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (3)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (5)
DataBucket(95-835)Containment(89-93)NullBucket(49-67)Bucket(70-70)SomeContainment(80-88)frontend/javascripts/oxalis/constants.ts (3)
Vector3(13-13)BucketAddress(22-24)Vector4(14-14)frontend/javascripts/libs/utils.ts (1)
mod(22-24)
🪛 GitHub Check: frontend-tests
frontend/javascripts/oxalis/api/api_latest.ts
[failure] 16-16:
Module '"libs/utils"' has no exported member 'Writeable'.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (16)
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (12)
2-2: Updated vector utility imports to include V4The import was updated to include V4 utilities, which are used for comparing 4D vectors in the bucket address comparison logic later in the code.
9-9: Addedmodimport from utilsThe
modutility function is now imported for handling bucket coordinate calculations correctly, especially important for partial bucket containment.
19-19: AddedVector4type importThe
Vector4type is now imported to properly type the zoomed addresses when comparing bucket addresses.
26-30: Improved type imports from the bucket moduleThe type imports have been expanded to include
ContainmentandSomeContainmentfrom the bucket module, which are essential for the new containment-aware functionality.
57-57: Updated data type from array to Map for better performanceThe
dataproperty inCubeEntrywas changed from an array to aMap<number, DataBucket>, which is more efficient for bucket lookups by index.
78-80: Added containment type constantsAdded
NoContainmentandFullContainmentconstants that represent the two basic containment types. These improve code readability and prevent repeatedly creating the same objects.
81-97: Added helpers for bucket coordinate calculationsNew helper functions have been added to manage containment-aware bucket coordinates:
zeroToBucketWidthconverts 0 toBUCKET_WIDTH, handling edge casesmakeLocalMincalculates local minimum coordinates with modulo arithmeticmakeLocalMaxcalculates local maximum coordinates with proper bucket width handlingThese functions support the partial containment calculation in the new
checkContainmentmethod.
236-285: Implemented containment-aware bucket checkingThe new
checkContainmentmethod replaces the simple boolean check with detailed containment checking. It determines whether a bucket:
- Has no overlap with the bounding box (
"no")- Is fully contained within the bounding box (
"full")- Is partially contained by the bounding box (
"partial")For partial containment, it also computes the local min/max coordinates that define the valid portion of the bucket. This is crucial for restricting voxel operations to only the valid parts of partially contained buckets.
350-371: Improved bucket address validation ingetBucketThe
getBucketmethod now double-checks that the bucket's stored address matches the requested address usingV4.isEqualto avoid collisions due to address linearization. This ensures that operations on out-of-bounds addresses don't affect other buckets accidentally.
336-347: Updated containment check ingetOrCreateBucketThe method now uses the new
checkContainmentfunction instead of a simple bounds check. It only creates buckets for addresses that have at least partial containment, returningNULL_BUCKETfor addresses with no containment.
373-389: Updated bucket creation to include containment informationThe
createBucketmethod now accepts and passes acontainmentparameter to theDataBucketconstructor, allowing buckets to know their containment status relative to the layer's bounding box. This enables buckets to restrict voxel operations to only the valid portions.
531-566: Clarified deprecation of_labelVoxelInResolution_DEPRECATEDThe method was updated with a clearer comment explaining that it should only be used for testing and should be replaced with the more efficient approach using
LabeledVoxelsMap. This aligns with the refactoring oflabelVoxelsin api_latest.ts.frontend/javascripts/oxalis/api/api_latest.ts (4)
24-24: Added LabeledVoxelsMap type importThis type is crucial for the refactored
labelVoxelsmethod which now uses a map of labeled voxels for batch processing.
73-73: Added enforceActiveVolumeTracing importThe new implementation of
labelVoxelsneeds to enforce that an active volume tracing exists, which is handled by this imported function.
136-136: Added imports for new voxel labeling functionalitySeveral new imports have been added to support the refactored
labelVoxelsmethod:
finishAnnotationStrokeActionto properly finish the annotation strokeDimensionsfor dimensional calculationszoomedPositionToZoomedAddressfor position-to-address conversionapplyLabeledVoxelMapToAllMissingMagsandapplyVoxelMapfor efficient voxel applicationThese imports provide the functionality needed for the containment-aware voxel labeling approach.
Also applies to: 145-145, 155-156, 160-161
2171-2280: Complete rewrite of labelVoxels methodThe
labelVoxelsmethod has been completely refactored from an asynchronous per-voxel approach to a synchronous batch processing approach. Key improvements:
- Uses
enforceActiveVolumeTracingto verify an active volume tracing- Groups voxels by their third dimension (z-slice) for efficient processing
- Creates containment-aware labeled voxel maps for each bucket
- Applies voxel maps respecting bucket containment boundaries
- Propagates changes across magnifications
- Dispatches appropriate actions to update the UI and finish the stroke
This implementation aligns with the PR's objective to ensure voxels outside the layer's bounding box cannot be mutated, as the containment-aware bucket system will restrict operations to valid regions.
…rder so that one can label the full bbox (and a bit more consequentially)
|
Thank you for your feedback! Could you have another look? :)
Good find 👍 I had a look and this only happens if the bbox is not aligned with the mag restriction. In that case, it's not possible to cut off the labeling at the exact border. Either, we crop too much or too little. Have a look at this screenshot where you can see what the smallest "voxel unit" is and that the border goes through it.
Therefore, I think, that it is alright to let the user brush a bit over the bbox limit as otherwise, there would be an area within the bbox which cannot be labeled. I had to adapt the rounding code so that it also works similarily for annotating at the bottom-right border. See my latest commit :) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (1)
21-22: 🛠️ Refactor suggestionRemove duplicate import of Constants
You're importing the default export from 'oxalis/constants' twice - once as
constantsand again asConstants. This is redundant and can confuse readers.import constants, { MappingStatusEnum } from "oxalis/constants"; -import Constants from "oxalis/constants";Then use
constantsinstead ofConstantsthroughout the code, or rename the first import:-import constants, { MappingStatusEnum } from "oxalis/constants"; -import Constants from "oxalis/constants"; +import Constants, { MappingStatusEnum } from "oxalis/constants";
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (1)
2-2: Minor optimization: Import specific symbols from 'libs/mjs'Instead of importing the entire
V3andV4modules, consider importing only the specific functions you need to reduce bundle size.-import { V3, V4 } from "libs/mjs"; +import { isEqual as V4isEqual, equals as V3equals, scale3, sub, add } from "libs/mjs";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/oxalis/api/api_latest.ts(12 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (3)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (4)
DataBucket(95-835)Containment(89-93)Bucket(70-70)SomeContainment(80-88)frontend/javascripts/oxalis/constants.ts (3)
Vector3(13-13)BucketAddress(22-24)Vector4(14-14)frontend/javascripts/libs/utils.ts (1)
mod(22-24)
🪛 GitHub Check: frontend-tests
frontend/javascripts/oxalis/api/api_latest.ts
[failure] 2009-2009:
Cannot find name 'Writeable'.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (18)
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (9)
9-9: Good improvement: Adding themodutility function for proper modulo operationsThis addition is a great improvement as JavaScript's default
%operator doesn't handle negative numbers as mathematical modulo. The importedmodutility ensures consistent behavior with both positive and negative inputs.
26-30: Good type imports for containment handlingThe addition of these types makes the code more maintainable by providing clear type definitions for bucket containment states.
78-79: Good use ofconstdeclarations for containment typesUsing const objects with the
as constassertion for the containment types ensures proper type inference and makes the code more maintainable.
81-97: Well-implemented helper functions for bounding box calculationsThese utility functions abstract complex logic for determining local bucket coordinates. The
zeroToBucketWidthfunction correctly handles the edge case where the modulo result is 0.
246-285: Excellent implementation of detailed containment checkingThis new
checkContainmentmethod improves on the previous boolean logic by providing detailed information about how a bucket intersects with the layer's bounding box. This is crucial for the PR's goal of limiting voxel modification to areas within the bounding box.The implementation correctly identifies three distinct states:
- No containment (
NoContainment)- Full containment (
FullContainment)- Partial containment with specific min/max coordinates
336-344: Improved bucket creation logic with containment awarenessThe updated
getOrCreateBucketnow properly checks containment status before creating a bucket, and passes the containment information to the bucket creation process. This ensures buckets are properly constrained to the bounding box.
350-371: EnhancedgetBucketwith improved address validationThe updated implementation adds an important check to verify that the bucket's stored address matches the requested address. This prevents potential issues when linearizing out-of-bounds addresses might collide with valid addresses.
Using
V4.isEqualfor the comparison is efficient and clear.
373-381: Updated bucket creation with containment awarenessThe
createBucketmethod now accepts and passes along the containment information, allowing buckets to know exactly which portions of their volume are within the layer's bounding box.
531-566: Clarified purpose of deprecated methodThe comment explaining that this method is only for testing and should be refactored away is valuable. It guides future developers to use the newer, more efficient approach with
LabeledVoxelsMap.frontend/javascripts/oxalis/api/api_latest.ts (9)
16-16: Good dependency update: Importing specific utilitiesImporting only the needed utilities
coalesceandmodis a clean approach that improves code maintainability.
24-24: Good dependency update: Adding LabeledVoxelsMap typeAdding this type import supports the refactored labelVoxels implementation that uses labeled voxel maps.
73-73: Useful addition of enforceActiveVolumeTracingImporting this utility function enables better validation and error handling in the refactored labelVoxels method.
136-136: Good addition of finishAnnotationStrokeActionThis import supports the stroke-based labeling approach in the refactored implementation.
145-145: Good import: Using named import for DimensionsUsing a named import for Dimensions rather than lowercase
dimensionsimproves consistency.
155-155: Additional position converter importAdding the
zoomedPositionToZoomedAddressimport is necessary for the refactored labelVoxels method.
160-161: Good imports for volume annotation functionalityThese imports provide the necessary functions for the refactored labelVoxels implementation, leveraging the updated containment-aware bucket handling.
422-436: Improved example formatting and clarityThe updated example comments are more consistently formatted, which improves the API documentation. The examples now follow a more consistent pattern.
2171-2280: Excellent refactoring of the labelVoxels methodThe completely rewritten
labelVoxelsmethod is a significant improvement over the previous implementation:
- It operates synchronously rather than asynchronously
- It processes all input voxels as a single brush stroke
- It groups voxels by the third dimension for efficient processing
- It creates labeled voxel maps and applies them to the segmentation cube
- It leverages the new containment-aware bucket handling to respect bounding box limits
This implementation aligns with the PR's goal of ensuring voxel labeling only happens within the layer's bounding box.
There was a problem hiding this comment.
Therefore, I think, that it is alright to let the user brush a bit over the bbox limit as otherwise, there would be an area within the bbox which cannot be labeled. I had to adapt the rounding code so that it also works similarily for annotating at the bottom-right border. See my latest commit :)
Sounds good as well as your latest changes.
Once the CI is green and this branch is up to date with the master I'll give this another quick re-test and then it should be ready to go 🟢
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
frontend/javascripts/viewer/model/bucket_data_handling/pullqueue.ts (1)
205-209: Good consolidation of cleanup operations.The new
destroy()method properly combines existing cleanup operations (clear()andabortRequests()) into a single method, ensuring they're always performed together. This supports the PR's overall goal of improving bucket and data management.Consider adding a JSDoc comment to document the purpose and behavior of this method:
+/** + * Destroys this pull queue by clearing it and aborting all pending requests. + * After calling this method, the queue should no longer be used. + */ destroy() { this.isDestroyed = true; this.clear(); this.abortRequests(); }CHANGELOG.unreleased.md (1)
34-34: LGTM: Good changelog entry for the bounding box fix.The changelog entry clearly describes the bugfix related to preventing voxel brushing outside of layer bounding boxes. This aligns well with the PR objectives.
You could consider simplifying "outside of" to just "outside" for conciseness:
- Voxels outside of the layer bounding box cannot be brushed, anymore. [#8602](https://github.com/scalableminds/webknossos/pull/8602) + Voxels outside the layer bounding box cannot be brushed, anymore. [#8602](https://github.com/scalableminds/webknossos/pull/8602)🧰 Tools
🪛 LanguageTool
[style] ~34-~34: This phrase is redundant. Consider using “outside”.
Context: ...bleminds/webknossos/pull/8599) - Voxels outside of the layer bounding box cannot be brushe...(OUTSIDE_OF)
frontend/javascripts/viewer/model/bucket_data_handling/bucket.ts (1)
127-133: Mandatorycontainmentargument may break external instantiations
DataBucketnow requires aSomeContainmentparameter. All internal
call-sites seem to be updated, but any downstream plugins or test helpers
that directly instantiatenew DataBucket(...)will now fail at compile
time.If binary compatibility is important, consider providing a default value
(e.g.{ type: "full" }) or exposing a convenience factory to construct a
bucket without having to know the containment details.frontend/javascripts/viewer/api/api_latest.ts (2)
2205-2213: Minor optimisation: skipmod()when indices are already in-range
aandboriginate fromMath.floor(pos[i] / labeledMag[i]), therefore
they are guaranteed to be>= 0. Using% Constants.BUCKET_WIDTH
directly is both clearer and ~15 % faster than an extra function call.-const localA = mod(a, Constants.BUCKET_WIDTH); -const localB = mod(b, Constants.BUCKET_WIDTH); +const localA = a % Constants.BUCKET_WIDTH; +const localB = b % Constants.BUCKET_WIDTH;This is obviously a micro-optimisation, but
labelVoxelsis called from
interactive brushing code; every allocation spared helps when thousands of
voxels are processed per frame.
2256-2266: Possible performance win: merge identicalUint8ArrayinstancesA new
Uint8Arrayis allocated per bucket per slice. For dense brushes
this can allocate hundreds of short-lived 1 KB buffers each stroke. You
can reuse a single zeroed array from a pool:const pooledLabelMap = new Uint8Array(Constants.BUCKET_WIDTH ** 2); // … pooledLabelMap.fill(0); // write voxels … currentLabeledVoxelMap.set(bucketZoomedAddress, pooledLabelMap);Allocation pressure drops and GC pauses during heavy painting sessions are
reduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/libs/utils.ts(1 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(1 hunks)frontend/javascripts/test/model/binary/cube.spec.ts(2 hunks)frontend/javascripts/test/model/binary/layers/wkstore_adapter.spec.ts(1 hunks)frontend/javascripts/test/model/binary/pullqueue.spec.ts(1 hunks)frontend/javascripts/test/model/binary/temporal_bucket_manager.spec.ts(4 hunks)frontend/javascripts/test/model/texture_bucket_manager.spec.ts(1 hunks)frontend/javascripts/test/model/volumetracing/volume_annotation_sampling.spec.ts(7 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts(1 hunks)frontend/javascripts/types/globals.d.ts(2 hunks)frontend/javascripts/viewer/api/api_latest.ts(12 hunks)frontend/javascripts/viewer/api/wk_dev.ts(2 hunks)frontend/javascripts/viewer/constants.ts(2 hunks)frontend/javascripts/viewer/model/bucket_data_handling/bounding_box.ts(3 hunks)frontend/javascripts/viewer/model/bucket_data_handling/bucket.ts(8 hunks)frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts(7 hunks)frontend/javascripts/viewer/model/bucket_data_handling/pullqueue.ts(3 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts
- frontend/javascripts/test/model/binary/pullqueue.spec.ts
- frontend/javascripts/viewer/api/wk_dev.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/javascripts/libs/utils.ts
- frontend/javascripts/types/globals.d.ts
- frontend/javascripts/test/helpers/apiHelpers.ts
- frontend/javascripts/test/model/binary/temporal_bucket_manager.spec.ts
- frontend/javascripts/test/model/texture_bucket_manager.spec.ts
- frontend/javascripts/test/model/binary/layers/wkstore_adapter.spec.ts
- frontend/javascripts/test/model/binary/cube.spec.ts
- frontend/javascripts/test/model/volumetracing/volume_annotation_sampling.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/viewer/api/api_latest.ts (4)
frontend/javascripts/types/globals.d.ts (1)
Writeable(25-25)frontend/javascripts/types/api_types.ts (1)
AdditionalCoordinate(30-30)frontend/javascripts/libs/utils.ts (1)
mod(22-24)frontend/javascripts/viewer/model/volumetracing/volume_annotation_sampling.ts (1)
applyVoxelMap(304-377)
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~34-~34: This phrase is redundant. Consider using “outside”.
Context: ...bleminds/webknossos/pull/8599) - Voxels outside of the layer bounding box cannot be brushe...
(OUTSIDE_OF)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (17)
frontend/javascripts/viewer/model/bucket_data_handling/pullqueue.ts (2)
34-34: Good addition of lifecycle tracking flag.The
isDestroyedflag provides a clear way to track the PullQueue's lifecycle state, allowing for proper cleanup and preventing operations after destruction.
109-111: Good defensive programming practice.Adding an early return when
isDestroyedis true prevents further processing of errors after the queue has been destroyed, which avoids potential issues with accessing or modifying destroyed objects.frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
178-180: LGTM: Guard clause prevents processing segment ID 0.Good addition of a guard clause to prevent processing segment ID 0. This is a safety check that helps maintain data integrity by avoiding updates to invalid segments.
frontend/javascripts/viewer/constants.ts (2)
22-24: Improve type safety with readonly BucketAddress.Adding
readonlyto the tuple types is a good defensive programming practice that prevents accidental mutations of bucket addresses, which helps maintain referential integrity throughout the application.
52-56: Improve type safety with const assertions for vector indices.Using
as constwith these vector indices arrays ensures type-level immutability and allows TypeScript to infer more precise types (readonly tuples of specific length instead of number[]). This change complements the other immutability improvements in this PR.frontend/javascripts/viewer/model/bucket_data_handling/bounding_box.ts (5)
5-5: Use default import instead of named import for constants.This change is correctly importing the constants module as a default import, which matches the export style in constants.ts.
14-21: LGTM: Simplified BoundingBox constructor.The constructor has been simplified using a cleaner approach with direct assignment instead of iterative updates. This makes the code more readable and likely more efficient.
41-68: LGTM: Added efficient bounding box creation from bucket addresses.The new static methods
fromBucketAddressandfromBucketAddressFastprovide a clean way to create bounding boxes from bucket addresses. The "fast" variant is particularly useful as it avoids unnecessary object allocations in performance-critical code paths.The calculation of bucket bounds in world space is now more efficient with precomputed scaled sizes.
84-103: LGTM: Optimized bounding box intersection with allocation-free variant.The refactored
intersectedWithmethod and the newintersectedWithFastmethod follow the same pattern of providing both a class-based and an allocation-free variant for flexibility. This is excellent for performance optimization in tight loops where allocation overhead matters.
1-268:✅ Verification successful
Verify the impact of removing containsBucket method.
The
containsBucketmethod was removed entirely from the BoundingBox class. Based on the PR objectives, this seems to be replaced by a more detailed containment state system elsewhere that distinguishes between full, partial, or no containment.
🏁 Script executed:
#!/bin/bash # Search for references to the removed containsBucket method rg "containsBucket" --type tsLength of output: 141
No production references to
containsBucketfoundA search only turned up a mock in a test, not any real calls:
- frontend/javascripts/test/model/binary/pullqueue.spec.ts: stubbed
containsBucket: vi.fn()Since no production code invokes
containsBucketonBoundingBox, its removal has no breaking impact.frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts (7)
78-84: ReturnConstants.BUCKET_WIDTHonly when strictly needed
zeroToBucketWidthreturnsBUCKET_WIDTHfor an input of0, but keeps any other value unchanged.
If the input originates frommod()it is always guaranteed to be in[0, BUCKET_WIDTH-1].
Calling the helper therefore introduces an extra branch per axis that will only ever be taken for the
single (rare) corner case wheremod()itself returned0.-const zeroToBucketWidth = (el: number) => { - return el !== 0 ? el : Constants.BUCKET_WIDTH; -}; +const zeroToBucketWidth = (el: number) => + el === 0 ? Constants.BUCKET_WIDTH : el;Very minor, but trimming the function to a single expression avoids the extra conditional and
slightly improves readability.
[ suggest_nitpick ]
85-97: Negative coordinates can leak throughmod()
makeLocalMin/Maxrely onmod()to normalise possibly negative values.
IfBoundingBox.mincan ever become negative (e.g. cropped datasets that are shifted into negative
space) we have to ensure thatmod()implements a mathematical modulo and not JavaScript’s%
remainder (which would keep the sign).
Otherwise the resulting local coordinates will be negative and violate the invariant
0 ≤ local < BUCKET_WIDTH, breaking bucket-internal indexing.Please double-check
libs/utils/mod:export const mod = (a: number, n: number) => ((a % n) + n) % n;If the helper is not implemented like above, a one-liner fix in
mod()will protect every call site.
[ request_verification ]
237-244: Early-out path duplicates work & allocates needlessly
getCubeKeyshort-circuits forallCoords == null, but still allocates a new
array viafilter().map()when a (possibly empty) array is passed.
Because this method is on the hot path of every bucket access, consider avoiding the two new arrays:-const relevantCoords = allCoords.filter((coord) => this.additionalAxes[coord.name] != null); -return [zoomStep, ...relevantCoords.map((el) => el.value)].join("-"); +if (!allCoords?.length) return `${zoomStep}`; +let key = `${zoomStep}`; +for (const c of allCoords) { + if (this.additionalAxes[c.name] != null) key += `-${c.value}`; +} +return key;This keeps GC pressure lower when the viewer is continuously drawing.
[ suggest_optional_refactor ]
246-285:checkContainmentdoes the job, but performs duplicate work & misses a non-overlap guard
getOrCreateCubeEntryalready performed agetMagByIndexlookup.
checkContainmentrepeats the same call, costing one extra hashmap read per bucket creation.
Capturing themagingetOrCreateCubeEntryand returning it alongside the cube would
eliminate the duplicate lookup.If
BoundingBox.intersectedWithFast()returns an invalid box on non-overlap
(min > maxon one axis), the subsequentintersectionBBox.min === intersectionBBox.max
check will not detect it and we will fall through to the partial case,
producing boguslocal min/max.
A rapid early-out improves robustness:+if (!areBoundingBoxesOverlappingOrTouching(this.boundingBox, bucketBBox)) { + return NoContainment; +}
- Minor naming:
FullContainmentandNoContainmentare constants, but
SomeContainmentis a type. Renaming the former toNO_CONTAINMENT/
FULL_CONTAINMENT(or vice-versa) makes the code self-explanatory.
[ suggest_essential_refactor ]
336-345:getOrCreateBucketcan skipcreateBucketfor truly empty partialsWhen
containment.type === "partial"we still allocate a full
DataBucket, even though the intersected volume might be entirely empty
(e.g. brush attempts that miss the bounding box by one voxel).
CheckingmakeLocalMin == makeLocalMaxwould let us
returnNULL_BUCKETinstead and avoid needless allocations:if ( containment.type === "partial" && V3.equals(containment.min, containment.max) ) { return this.getNullBucket(); }This optimisation keeps the bucket count lower when a user draws along the edge of
the dataset.
[ suggest_optional_refactor ]
356-365: Address collision guard is helpful but expensiveThe slice +
V4.isEqualcomparison protects against collisions across different
additional-coordinate cubes, yet runs on every successful lookup.
Because collisions can only happen for out-of-bbox addresses (which are rare),
consider reversing the logic: compare only whencubeKeycomes from an unknown
cube (i.e. whencube == null, the case we already treat separately), or cache a
BooleanisOutsideBoundingBoxflag on the bucket at construction time and skip
the equality check forfalse.
[ suggest_optional_refactor ]
538-542: Deprecation comment – please add TODO with ticketThe note clarifies that
_labelVoxelInResolution_DEPRECATEDis test-only and
should be removed, but without a tracking ticket it may linger indefinitely.
Appending a TODO with a link to an issue (or opening one) makes the clean-up
actionable.
[ suggest_nitpick ]
| globalPositionsMag1: Vector3[], | ||
| segmentId: number, | ||
| optAdditionalCoordinates?: AdditionalCoordinate[] | null, | ||
| ) { | ||
| const state = Store.getState(); | ||
| const allowUpdate = state.annotation.restrictions.allowUpdate; | ||
| const additionalCoordinates = | ||
| optAdditionalCoordinates === undefined | ||
| ? state.flycam.additionalCoordinates | ||
| : optAdditionalCoordinates; | ||
| if (!allowUpdate || globalPositionsMag1.length === 0) return; | ||
|
|
There was a problem hiding this comment.
Guard against missing finest magnification (zoom step 0)
magInfo.getClosestExistingIndex(0) is assumed to return a number, but the
implementation can return null if the dataset does not offer a mag at
zoom-step 0. In that case the subsequent call
magInfo.getMagByIndexOrThrow(labeledZoomStep) will throw.
const labeledZoomStep = magInfo.getClosestExistingIndex(0);
if (labeledZoomStep == null) {
throw new Error("No magnification level corresponding to zoom-step 0 found.");
}Failing fast with a clear message prevents the user from seeing an
ambiguous runtime crash later on.
it's green now :) |
There was a problem hiding this comment.
Hej I just gave this another test and it doesn't seem to work correctly anymore, or I am testing it wrong 🤔
Here is a screenshot of my test:
I am using l4_sample for testing with ds layer bounds:
3090, 3090, 512, 900, 900, 900-> segmentation3072, 3072, 512, 1024, 1024, 1024(original) -> color
As you can see in the screenshot, I am able to draw beyond the segmentation layers bounds and as I understood this PR, this shouldn't be possible.
…on tracings (in case of an existing fallback segmentation, that layer should be used)
|
Thanks for raising this! This is caused because the color layer was bigger than the segmentation layer. I tracked down the bug and found out that the bounding box of the segmentation tracing was always set to the union of all layer bboxes. However, this should only happen for empty volume tracings (without fallback). I pushed a fix. Please have another look :) |
There was a problem hiding this comment.
Sorry, but I am not sure about the latest change. The type ServerVolumeTracing itself should already have a bounding box which should be used as the source of truth for the annotation bbox limit IMO.
#7580 fixes the behaviour for the volume tracing bbox for explorations. So just using tracing.boundingBox in the model_init should be ok here imo.
Sorry for the extra round
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
35-37: Refine changelog entry wording for clarity and consistency.The phrase “outside of” is redundant and the comma before “anymore” is unnecessary. Consider rephrasing to match the style of other entries and improve readability:
- Voxels outside of the layer bounding box cannot be brushed, anymore. [#8602] + Voxels outside the layer bounding box can no longer be brushed. [#8602]🧰 Tools
🪛 LanguageTool
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...bleminds/webknossos/pull/8599) - Voxels outside of the layer bounding box cannot be brushe...(OUTSIDE_OF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...bleminds/webknossos/pull/8599) - Voxels outside of the layer bounding box cannot be brushe...
(OUTSIDE_OF)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
This PR takes care that buckets that are only partially contained by the layer's bbox can only be mutated within the area that is included in the bbox.
Additionally, it refactors the api
labelVoxelsfunction to not use deprecated code anymore.URL of deployed dev instance (used for testing):
Steps to test:
webknossos.DEV.api.data.labelVoxels([[1,1,1], [1,2,1], [2,1,1], [2,2,1]], 1337);)Issues:
(Please delete unneeded items, merge only when none are left open)