Support rendering more dtypes #8325
Conversation
📝 WalkthroughWalkthroughThis large pull request makes extensive adjustments across the frontend and backend code. In the JavaScript code, many method signatures and type definitions are updated—such as replacing union types with a generalized TypedArray and switching from Vector2 to tuple types for intensity ranges—while texture initialization, uniform handling, and error message construction are streamlined. Shader modules and test cases are refactored to remove redundant parameters and improve clarity. In addition, multiple Scala files in the datastore and tracingstore have migrated from using UnsignedInteger types to a new SegmentInteger type, refining data conversion and histogram logic. Changes
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
…ds are handled as if they would be positive)
...ossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (1)
82-99: Consider tightening defaultIntensityRange for 64-bit types.
Currently,uint64/int64both default to a[0..255]range, relying on the fact that segmentation layers rarely call this method. You could optionally throw an error or log a warning if 64-bit segmentation data callsdefaultIntensityRangeby mistake.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/controllers/AnnotationIOController.scala(1 hunks)app/models/annotation/AnnotationService.scala(3 hunks)app/models/annotation/nml/NmlParser.scala(2 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/ProtoGeometryImplicits.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala(5 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/RemoteFallbackLayer.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala(4 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/ProtoGeometryImplicits.scala
- webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala
- app/models/annotation/AnnotationService.scala
🧰 Additional context used
🧠 Learnings (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (2)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8325
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala:80-93
Timestamp: 2025-03-12T13:04:06.063Z
Learning: In WebKnossos, uint64 and int64 element classes are only supported for segmentation layers, where the frontend uses a fixed range of 0-255, and defaultIntensityRange should not be called for these types.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8325
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala:80-93
Timestamp: 2025-03-12T13:04:06.063Z
Learning: In WebKnossos, the full range of 64-bit integers (-2^63 to 2^63-1) is used in the frontend for int64 layers, with potential precision loss being accepted as an unsupported value range.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (21)
app/controllers/AnnotationIOController.scala (2)
336-339: Improved element class handlingThe code now uses
ElementClass.toProtoinstead of direct assignment, and wraps the result in aFoxmonad with.toFox. This change aligns with the PR objective of supporting additional data types (int8, int16, int32, int64) for rendering.
343-343: Variable name updated to match its contentThe variable name change from
elementClasstoelementClassProtomore accurately describes that the value is a protobuf representation of the element class.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala (8)
224-227: Well-aligned type migration for expanded dtype supportThe code has been updated to use
SegmentIntegerArrayandSegmentIntegerinstead ofUnsignedIntegerArrayandUnsignedInteger. This change supports the PR objective of rendering more data types, including signed integers.
757-758: Clear and appropriate variable renamingRenaming from
elementClasstoelementClassProtoimproves code clarity by explicitly indicating that this is a protobuf representation.
786-786: Consistent variable updateThe updated constructor call properly uses the renamed
elementClassProtovariable.
796-800: Consistent variable update in error messageThe error message reporting now correctly uses the renamed
elementClassProtovariable, maintaining consistency throughout the codebase.
819-820: Consistent parameter updateThe
saveBucketcall has been updated to use the renamedelementClassProtoparameter.
830-831: Consistent parameter updateThe
updateSegmentIndexcall has been updated to use the renamedelementClassProtoparameter.
905-905: Support for signed integer valuesChanged from
toPositiveLongtotoLong, which correctly allows for negative segment IDs. This is essential for supporting signed integer types as mentioned in the PR objectives.
920-923: Code structure improvementThe code structure has been slightly improved, making the flow more readable while maintaining the same functionality.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/RemoteFallbackLayer.scala (3)
11-11: Import added for ElementClassThe import for
ElementClasshas been added to support the new element class conversion functionality in thefromDataLayerAndDataSourcemethod. This is a necessary addition for the enhanced dtype support.
14-14: Box import added for error handlingThe import for
net.liftweb.common.Boxhas been added to support the return type change offromDataLayerAndDataSource. This enables proper error handling when element class conversion fails.
24-28:✅ Verification successful
Improved error handling in fromDataLayerAndDataSource method
The method signature has been changed from returning
RemoteFallbackLayertoBox[RemoteFallbackLayer], which is a significant improvement for error handling. The implementation now properly handles potential failures in element class conversion by using the Box monadic container.This change aligns with the PR objective of supporting more data types, as it allows graceful handling of conversion failures for the newly supported dtypes (uint32, int8, int16, int32, int64).
🏁 Script executed:
#!/bin/bash # Check for any places that call this method to ensure they handle the new Box return type rg "fromDataLayerAndDataSource" --type scala -A 3 -B 3Length of output: 2428
Verified: Improved Error Handling for
fromDataLayerAndDataSourceThe update changing the method’s return type from
RemoteFallbackLayertoBox[RemoteFallbackLayer]has been confirmed. This change correctly leverages the Box monadic container to gracefully handle conversion failures (for the new supported data types such as uint32, int8, int16, int32, int64). Moreover, our verification shows that the call site inVolumeTracingService.scalaproperly processes the Box result by chaining with.toFox.
- The method conversion now safely wraps potential failures.
- Callers (e.g., in
VolumeTracingService.scala) correctly handle the Box type.- The implementation aligns with the PR objective for enhanced error management.
Approving these changes.
app/models/annotation/nml/NmlParser.scala (1)
12-12: LGTM: Addition of ElementClassProto import.This import facilitates the transition from using ElementClass to ElementClassProto, which aligns with the PR's objective of supporting additional data types for rendering.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (7)
8-8: Import usage confirmed.
This new import forElementClassProtois clearly needed for the addedfromProtoandtoProtofunctions.
26-26: New imports for Liftweb components are valid.
These imports (Box,Failure,Full) are suitable for the newly introduced return types intoProtoand error handling.
48-48: Expanded segmentationElementClasses set.
Includingint8,int16,int32, andint64aligns with the PR objective of supporting more signed integer types for segmentation.
67-80: isSigned method appears consistent.
Treating float/double as “true” for signedness is reasonable. The mapping covers all element classes without gaps.
115-128: fromProto mapping is straightforward.
All new integer element classes are handled. The thrown exception for unrecognized values is appropriate.
130-141: Protecting toProto from unsupported classes.
Returning aFailureif the element class is not mapped prevents hidden errors. Consider clarifying float/double usage in the future if needed.
145-152: Validate whether negative IDs should be allowed.
The code sets max segment IDs for signed types yet also assumeslargestSegmentId >= 0. If negative segment IDs are truly unsupported, this is fine. Otherwise, the system might erroneously reject valid negative IDs.
| editPosition = nmlParams.editPosition, | ||
| editRotation = nmlParams.editRotation, | ||
| elementClass = ElementClass.uint32, // Note: this property may be adapted later in adaptPropertiesToFallbackLayer | ||
| elementClass = ElementClassProto.uint32, // Note: this property may be adapted later in adaptPropertiesToFallbackLayer |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider dynamic element class assignment instead of hardcoding to uint32.
While changing to ElementClassProto.uint32 is consistent with the import change, the PR objective is to support multiple data types (int8, int16, int32, int64, etc.). Hardcoding to uint32 might limit the flexibility needed for the enhanced data type support.
🏁 Script executed:
#!/bin/bash
# Check if there are any other places where ElementClassProto is used dynamically
# Look for ElementClassProto references
rg -A 5 "ElementClassProto" --type scala
# Check if there is any code that determines element class based on data type
rg -A 10 "elementClass =" --type scalaLength of output: 56119
Action Required: Use Dynamic Element Class Assignment in NmlParser.scala
The current implementation in NmlParser.scala (line 78) still hardcodes elementClass as ElementClassProto.uint32. Given that the PR aims to support multiple data types (e.g., int8, int16, int32, int64, etc.), this reduces flexibility and may cause inconsistencies with other parts of the codebase that derive the element class dynamically.
- File: app/models/annotation/nml/NmlParser.scala
Line: 78
Issue:elementClassis hardcoded instead of being computed based on context (for example, using input parameters or configuration similar to dynamic assignments in VolumeTracing or DataLayer modules).
Please consider refactoring the assignment to derive elementClass dynamically and ensure it aligns with the enhanced data type support objective. Alternatively, add a clear justification if dynamic assignment is to be deferred.
There was a problem hiding this comment.
Backend LGTM, retest looked good.
Could not test segment stats since there is no signed int segmentation dataset with a segment index file. Do you know if the segment index file format would support this, especially with negative IDs? Otherwise the job to compute one might fail for these segmentation layers. Same question for the compute meshes job.
I guess from now on we need to test all new segmentation/volume/mapping/proofreading/mesh features also with segmentations with negative ids… Admittedly, such datasets are rarely used by users and maybe we can fix some issues later as they occur.
How can I trigger the index computation? Is it done on DS upload (if the worker is set up)? my suggestion would be to test this once it's deployed and then schedule a follow up if necessary.
I think, we don't necessarily need to add this to our testing agenda. as you say, such DS should be very rare and the current PR ensures that at least basic functionality (like rendering, annotating and ad hoc meshing etc) works. if users run into problems, we can fix that ad hoc. prior to this PR, DS with negative ids wouldn't work either, so it's still an improvement. |
I think it’s an item in the layer sidebar for segmentation layers. There is a function
Ok, agreed! |
|
Wasn't the If I recall correctly some months ago you deprecated one of the jobs @fm3 |
That would probably be |
There was a problem hiding this comment.
Latest frontend changes lgtm. Is retesting the fixes needed before merging or did you do that already @philippotto?
I'll do that before merging. thanks for the approval! |
This PR adds rendering support for uint32 color layers, int8, int16, int32 for color + segmentation, and int64 for segmentation layers.
Histogram and thumbnail support were only done partially. The rest can happen in another PR.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)