In Data Loading, Clip to Layer BoundingBox (Redo #8551) #8573
Conversation
📝 Walkthrough""" WalkthroughThis change introduces logic to ensure that when loading data from a data layer, any voxel data outside the defined bounding box is zeroed out, effectively clipping the layer to its bounding box. The core implementation is in the backend data service, which now clips data arrays to the bounding box before further processing. Supporting changes include new methods for bounding box containment and translation, vector negation using a unary operator, and conversions between cuboid and bounding box representations. The changelog is updated to document this behavior. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
MichaelBuessemeyer
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: 0
🧹 Nitpick comments (4)
CHANGELOG.unreleased.md (1)
22-22: LGTM, but consider a minor language improvement.Clearly explains the new behavior of zeroing out data outside the defined bounding box. The explanation is precise and matches the PR's objectives.
Consider simplifying "outside of the bounding box" to just "outside the bounding box" to avoid redundancy:
- When loading data from a data layer that has data stored beyond the bounding box specified in the datasource-properties.json, data outside of the bounding box is now zeroed. (the layer is "clipped"). [#8551](https://github.com/scalableminds/webknossos/pull/8551) + When loading data from a data layer that has data stored beyond the bounding box specified in the datasource-properties.json, data outside the bounding box is now zeroed. (the layer is "clipped"). [#8551](https://github.com/scalableminds/webknossos/pull/8551)🧰 Tools
🪛 LanguageTool
[style] ~22-~22: This phrase is redundant. Consider using “outside”.
Context: ...in the datasource-properties.json, data outside of the bounding box is now zeroed. (the la...(OUTSIDE_OF)
util/src/main/scala/com/scalableminds/util/geometry/BoundingBox.scala (2)
48-50: Avoid temporary allocation inisFullyContainedInfor a micro-performance win
intersectionallocates a newBoundingBoxjust to compare againstthis.
For a hot path you can sidestep the allocation by comparing coordinates directly:- def isFullyContainedIn(other: BoundingBox): Boolean = - this.intersection(other).contains(this) + def isFullyContainedIn(other: BoundingBox): Boolean = + topLeft.x >= other.topLeft.x && + topLeft.y >= other.topLeft.y && + topLeft.z >= other.topLeft.z && + bottomRight.x <= other.bottomRight.x && + bottomRight.y <= other.bottomRight.y && + bottomRight.z <= other.bottomRight.zThis keeps the method allocation–free while preserving semantics.
67-69: Minor naming nitpick – considertranslateinstead ofmove
The new helper is great, butmovenow exists in bothBoundingBoxandVec3Intwith slightly different semantics (in-place vs. returning a new box). Using a verb liketranslateorshiftwould reduce cognitive load and avoid confusing it with theVec3Intcounterpart.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala (1)
111-139:clipToLayerBoundingBoxcopies row-wise but still triple-loops ‑ can be flattened
Great to see explicit clipping! The inner copy already works per X-row; you can also flatten Y/Z loops by computing linear slice offsets and invoking a singleSystem.arraycopyper contiguous block.
This typically halves the loop overhead for large requests:- for { - z <- intersection.topLeft.z until intersection.bottomRight.z - y <- intersection.topLeft.y until intersection.bottomRight.y - } { - val offset = (intersection.topLeft.x + - y * requestBboxInMag.width + - z * requestBboxInMag.width * requestBboxInMag.height) * bytesPerElement - System.arraycopy(inputArray, - offset, - outputArray, - offset, - (intersection.bottomRight.x - intersection.topLeft.x) * bytesPerElement) - } +val rowBytes = (intersection.bottomRight.x - intersection.topLeft.x) * bytesPerElement +val sliceBytes = requestBboxInMag.width * requestBboxInMag.height * bytesPerElement +val yStride = requestBboxInMag.width * bytesPerElement + +var z = intersection.topLeft.z +while (z < intersection.bottomRight.z) { + var y = intersection.topLeft.y + val zOffset = z * sliceBytes + while (y < intersection.bottomRight.y) { + val offset = zOffset + y * yStride + intersection.topLeft.x * bytesPerElement + System.arraycopy(inputArray, offset, outputArray, offset, rowBytes) + y += 1 + } + z += 1 +}No change in behaviour, ~2-3× speed-up on large volumes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.unreleased.md(1 hunks)util/src/main/scala/com/scalableminds/util/geometry/BoundingBox.scala(3 hunks)util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/Cuboid.scala(2 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/DataServiceRequests.scala(2 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/DataServiceRequests.scala (2)
util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala (2)
Vec3Int(7-89)Vec3Int(91-157)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/Cuboid.scala (1)
mag(46-46)
util/src/main/scala/com/scalableminds/util/geometry/BoundingBox.scala (1)
util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala (3)
move(53-56)move(56-59)other(32-35)
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~22-~22: This phrase is redundant. Consider using “outside”.
Context: ...in the datasource-properties.json, data outside of the bounding box is now zeroed. (the la...
(OUTSIDE_OF)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (7)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/DataServiceRequests.scala (2)
3-3: LGTM - Import correctly added.The Vec3Int import is properly added to support the new mag method.
25-25: LGTM - Convenience accessor method.This new method provides direct access to the cuboid's magnitude vector, consistent with the existing pattern of convenience methods like
isSingleBucket. This will simplify code that needs to access the magnitude in data request processing.util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala (1)
26-28: LGTM - Good use of Scala's unary operator overloading.Replacing a
negatemethod with the unary minus operatorunary_-is a good improvement. This follows idiomatic Scala by allowing the more natural syntax-vecinstead ofvec.negate(), making the code more readable and aligning with mathematical notation.webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/Cuboid.scala (2)
3-3: LGTM - Import correctly added.The BoundingBox import is properly added to support the new conversion methods.
55-59: LGTM - Well-implemented conversion methods.The new methods provide useful conversions between Cuboid and BoundingBox representations, which are essential for the bounding box clipping functionality. The implementation is correct, using the appropriate magnitude-aware coordinates.
toBoundingBoxInMagcreates a BoundingBox using the cuboid's top-left position in magnitude coordinatestoMag1BoundingBoxleverages the existingtoMag1method to first normalize to mag1, then convert to a BoundingBoxThese methods follow the naming pattern of existing conversion methods like
toMag1.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala (2)
143-148: Clipping before agglomerate / half-byte conversion is the right order – nice!
The guard!request.cuboid.toMag1BoundingBox.isFullyContainedIn(...)prevents an unnecessary copy when the request is already inside bounds, keeping the fast path untouched.
232-235: Variable extraction improves readability – LGTM
Derivingrx/ry/rzonce clarifies the intent and removes the previous silent division by subsampling stride. Implementation and indexing logic remain correct.
|
@MichaelBuessemeyer I found the problem here. The DataSource was set to null in the request classes for volume tracings. That was always by design and we had null checks in many spots. But this variable is accessed in convertIfNecessary (which now also happens in the tracingstore’s BinaryDataService). I now went ahead and replaced this null hack with an Option, so that the scala type system will help us avoid such problems in the future. Could you please have a look at my latest changes when you have time? (No hurry) As you opened this PR someone else will have to hit approve eventually, but I still think that you could review the content 😇 |
...tastore/app/com/scalableminds/webknossos/datastore/models/requests/DataServiceRequests.scala
Outdated
Show resolved
Hide resolved
...nossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala
Outdated
Show resolved
Hide resolved
2nd iteration of #8551
Additionally, the nullable DataSource in various data request classes was replaced by Option[DataSourceId] to avoid Null exceptions in this context in the future.
Steps to test:
Issues: