In Data Loading, Clip to Layer BoundingBox #8551
Conversation
📝 Walkthrough""" WalkthroughThis change introduces logic to ensure that data returned from a data layer is clipped to its defined bounding box. Specifically, when a data request includes regions outside the layer’s bounding box, those out-of-bounds regions are zeroed out before further processing. This is implemented by adding a clipping function in the backend service, modifying the data conversion pipeline to apply this clipping step, and updating related geometry utility classes to facilitate bounding box operations. Additional utility methods are added to support bounding box containment checks and coordinate transformations. Changes
Assessment against linked issues
Poem
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 (
|
![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 (3)
CHANGELOG.unreleased.md (1)
16-16: Minor language improvement suggestion.Consider simplifying "outside of the bounding box" to "outside the bounding box" to remove 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] ~16-~16: 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)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/Cuboid.scala (1)
55-60: Document coordinate system & inclusive/exclusive semanticsThese helpers are valuable, but future maintainers need to know
- whether
bottomRightis treated as inclusive or exclusive, and- whether
width/height/depthhere are in current‑mag voxel units or already scaled.Adding a brief Scaladoc (1‑2 lines) to
toBoundingBoxInMag/toMag1BoundingBoxwould prevent off‑by‑one misunderstandings later.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala (1)
109-137: Skip allocation when nothing needs to be clipped
clipToLayerBoundingBoxallocates a full‑sized zero buffer even if the intersection coincides with the request cuboid (possible whenisFullyContainedInwas overly conservative due to rounding).A cheap guard avoids the extra work:
@@ val outputArray = Array.fill[Byte](inputArray.length)(0) - intersectionOpt.foreach { intersection => + intersectionOpt match { + case Some(intersection) if + intersection.width == requestBboxInMag.width && + intersection.height == requestBboxInMag.height && + intersection.depth == requestBboxInMag.depth => + // Nothing to clip – reuse original array + return Full(inputArray) + case Some(intersection) => for { z <- intersection.topLeft.z until intersection.bottomRight.z @@ - } + } + case None => // keep all zeros (completely outside) +}This keeps memory steady and avoids an unnecessary copy in borderline cases.
📜 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] ~16-~16: 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: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (7)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/DataServiceRequests.scala (1)
3-3: Accessor method looks good.The addition of the
magmethod provides convenient access to the cuboid's magnitude vector, maintaining a consistent pattern of delegating geometric information through the object hierarchy.Also applies to: 25-25
util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala (1)
26-28: Good choice replacing with standard Scala operator.Replacing a custom
negatemethod with the standard Scala unary negation operatorunary_-improves code readability and follows idiomatic Scala conventions. This allows for more natural syntax when negating vectors (using-vecinstead of a method call).util/src/main/scala/com/scalableminds/util/geometry/BoundingBox.scala (3)
9-9: Good optimization with lazy evaluation.Making
bottomRighta lazy val is a good optimization, especially if this field isn't always accessed. It defers the computation until the first time the field is accessed, potentially improving performance.
48-50: Elegant containment check implementation.The
isFullyContainedInmethod provides a clean way to check if a bounding box is fully contained within another, leveraging the existingintersectionmethod. This approach is both concise and logically clear.
67-69: Efficient implementation of move operation.The
movemethod provides a convenient way to translate a bounding box by a vector delta. Usingcopywith just thetopLeftparameter is an efficient approach that maintains immutability while only modifying the necessary field.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala (2)
141-146: Verify mixed coordinate spaces in containment check vs. clippingContainment is evaluated in mag‑1 space (
toMag1BoundingBox) whereas clipping operates in the request’s own mag (toBoundingBoxInMag).
When integer division rounds coordinates differently across mags, a cuboid might be considered fully contained at mag‑1 yet still leak a few voxels outside the layer bbox at the coarser mag.If the asymmetry is deliberate, please add an explanatory comment; otherwise align both steps to the same coordinate system.
227-230: Cleaner offset computation – looks goodSwapping the division‑based offsets for straight coordinate deltas (
rx,ry,rz) removes dead code (subsamplingStrides) and clarifies intent. Nice simplification.
fm3
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
18-18: Refine phrasing for clarity and concisenessConsider removing the redundant “of” in “outside of the bounding box” and streamlining the sentence. Also, replace the parentheses with a more integrated explanation. For example:
- - 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) + - For data layers storing voxels beyond the bounding box defined in `datasource-properties.json`, any voxel data outside the bounding box is now zeroed out (clipped). [#8551](https://github.com/scalableminds/webknossos/pull/8551)🧰 Tools
🪛 LanguageTool
[style] ~18-~18: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~18-~18: 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
21-21: Refine phrasing to improve clarity and remove redundancy.The phrase “outside of” is redundant; use “outside”. Also consider folding the parenthetical into the main sentence for smoother reading.
Apply this diff:
- 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] + When loading data from a data layer that has data stored beyond the bounding box specified in datasource-properties.json, data outside the bounding box is now zeroed, effectively clipping the layer. [#8551]🧰 Tools
🪛 LanguageTool
[style] ~21-~21: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~21-~21: 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: frontend-tests
- GitHub Check: backend-tests
This reverts commit 7275b8b.
| @@ -51,4 +51,10 @@ case class Cuboid(topLeft: VoxelPosition, width: Int, height: Int, depth: Int) { | |||
| height * mag.y, | |||
| depth * mag.z | |||
| ) | |||
|
|
|||
| def toBoundingBoxInMag: BoundingBox = | |||
| BoundingBox(Vec3Int(topLeft.voxelXInMag, topLeft.voxelYInMag, topLeft.voxelZInMag), width, height, depth) | |||
There was a problem hiding this comment.
shouldn't width, height, depth also be in mag?
There was a problem hiding this comment.
they already are, they are just not named as explicitly
| System.arraycopy(inputArray, | ||
| offset, | ||
| outputArray, | ||
| offset, | ||
| (intersection.bottomRight.x - intersection.topLeft.x) * bytesPerElement) |
There was a problem hiding this comment.
IMO this is the most likely cause for the nullpointer
There was a problem hiding this comment.
some out of bounds maybe. Maybe even due to my comment above regarding them dimensions not being converted to the respective mag
There was a problem hiding this comment.
For the record, the null was explicitly set for request.dataSource, compare #8573 (comment)
Steps to test:
Issues: