Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send baseMappingName when forwarding FullMeshRequest from TS to DS #7887

Merged
merged 3 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Fixed a bug where sometimes old mismatching javascript code would be served after upgrades. [#7854](https://github.com/scalableminds/webknossos/pull/7854)
- Fixed a bug where dataset uploads of zipped tiff data via the UI would be rejected. [#7856](https://github.com/scalableminds/webknossos/pull/7856)
- Fixed a bug with incorrect valiation of layer names in the animation modal. [#7882](https://github.com/scalableminds/webknossos/pull/7882)
- Fixed a bug in the fullMesh.stl route used by the render_animation worker job, where some meshes in proofreading annotations could not be loaded. [#7887](https://github.com/scalableminds/webknossos/pull/7887)

### Removed
- If the datasource-properties.json file for a dataset is missing or contains errors, WEBKNOSSOS no longer attempts to guess its contents from the raw data. Exploring remote datasets will still create the file. [#7697](https://github.com/scalableminds/webknossos/pull/7697)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class DSMeshController @Inject()(
the oversegmentation. Collect mesh chunks of all *unmapped* segment ids
belonging to the supplied agglomerate id.
If it is not set, use meshfile as is, assume passed id is present in meshfile
Note: in case of an editable mapping, targetMappingName is its baseMapping name.
*/
targetMappingName: Option[String],
editableMappingTracingId: Option[String]): Action[ListMeshChunksRequest] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,55 +23,59 @@ trait MeshMappingHelper {
mappingNameForMeshFile: Option[String],
omitMissing: Boolean, // If true, failing lookups in the agglomerate file will just return empty list.
token: Option[String])(implicit ec: ExecutionContext): Fox[List[Long]] =
targetMappingName match {
case None =>
(targetMappingName, editableMappingTracingId) match {
case (None, None) =>
// No mapping selected, assume id matches meshfile
Fox.successful(List(agglomerateId))
case Some(mappingName) if mappingNameForMeshFile.contains(mappingName) =>
case (Some(mappingName), None) if mappingNameForMeshFile.contains(mappingName) =>
// Mapping selected, but meshfile has the same mapping name in its metadata, assume id matches meshfile
Fox.successful(List(agglomerateId))
case Some(mappingName) =>
case (Some(mappingName), None) =>
// Mapping selected, but meshfile does not have matching mapping name in its metadata,
// assume agglomerate id, fetch oversegmentation segment ids for it
val agglomerateFileKey = AgglomerateFileKey(
organizationName,
datasetName,
dataLayerName,
mappingName
)
editableMappingTracingId match {
case Some(tracingId) =>
for {
tracingstoreUri <- dsRemoteWebknossosClient.getTracingstoreUri
segmentIdsResult <- dsRemoteTracingstoreClient.getEditableMappingSegmentIdsForAgglomerate(tracingstoreUri,
tracingId,
agglomerateId,
token)
segmentIds <- if (segmentIdsResult.agglomerateIdIsPresent)
Fox.successful(segmentIdsResult.segmentIds)
else
for {
agglomerateService <- binaryDataServiceHolder.binaryDataService.agglomerateServiceOpt.toFox
localSegmentIds <- agglomerateService.segmentIdsForAgglomerateId(
agglomerateFileKey,
agglomerateId
)
} yield localSegmentIds
} yield segmentIds
case _ =>
for {
agglomerateService <- binaryDataServiceHolder.binaryDataService.agglomerateServiceOpt.toFox
segmentIdsBox <- agglomerateService
.segmentIdsForAgglomerateId(
AgglomerateFileKey(
organizationName,
datasetName,
dataLayerName,
mappingName
),
Comment on lines +40 to +45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a duplication of this AgglomerateFileKey constructer (see lines 69ff.). Prior to these changes there was only one definition of the constructor and its result was stored in a val agglomerateFileKey.

=> A minor suggestion from my side would be to restore this and only have code that constructs the AgglomerateFileKey once :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! Yes, I have already considered this. However, I ultimately decided against it, since the only ways that I saw to deduplicate this (without completely changing the logic again) would be either to construct the AgglomerateFileKey above the whole match statement, but that doesn’t seem right, since in most cases it won’t be used, or to extract it to another function, but that doesn’t seem right, because its call would look exactly the same as the direct constructor call.

So I tend to think in this case breaking the DRY-rule is less bad than the alternatives :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, sounds like a good reason to me 👍

agglomerateId
)
.futureBox
segmentIds <- segmentIdsBox match {
case Full(segmentIds) => Fox.successful(segmentIds)
case _ => if (omitMissing) Fox.successful(List.empty) else segmentIdsBox.toFox
}
} yield segmentIds
case (Some(mappingName), Some(tracingId)) =>
// An editable mapping tracing id is supplied. Ask the tracingstore for the segment ids. If it doesn’t know,
// use the mappingName (here the editable mapping’s base mapping) to look it up from file.
for {
tracingstoreUri <- dsRemoteWebknossosClient.getTracingstoreUri
segmentIdsResult <- dsRemoteTracingstoreClient.getEditableMappingSegmentIdsForAgglomerate(tracingstoreUri,
tracingId,
agglomerateId,
token)
segmentIds <- if (segmentIdsResult.agglomerateIdIsPresent)
Fox.successful(segmentIdsResult.segmentIds)
else // the agglomerate id is not present in the editable mapping. Fetch its info from the base mapping.
for {
agglomerateService <- binaryDataServiceHolder.binaryDataService.agglomerateServiceOpt.toFox
segmentIdsBox <- agglomerateService
.segmentIdsForAgglomerateId(
agglomerateFileKey,
agglomerateId
)
.futureBox
segmentIds <- segmentIdsBox match {
case Full(segmentIds) => Fox.successful(segmentIds)
case _ => if (omitMissing) Fox.successful(List.empty) else segmentIdsBox.toFox
}
} yield segmentIds
}
localSegmentIds <- agglomerateService.segmentIdsForAgglomerateId(
AgglomerateFileKey(
organizationName,
datasetName,
dataLayerName,
mappingName
),
agglomerateId
)
} yield localSegmentIds
} yield segmentIds
case _ => Fox.failure("Cannot determine segment ids for editable mapping without base mapping")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ class TSRemoteDatastoreClient @Inject()(
def querySegmentIndex(remoteFallbackLayer: RemoteFallbackLayer,
segmentId: Long,
mag: Vec3Int,
mappingName: Option[String],
mappingName: Option[String], // should be the baseMappingName in case of editable mappings
editableMappingTracingId: Option[String],
userToken: Option[String]): Fox[Seq[Vec3Int]] =
for {
Expand All @@ -152,12 +152,13 @@ class TSRemoteDatastoreClient @Inject()(
indices = positions.map(_.scale(1f / DataLayer.bucketLength)) // Route returns positions to use the same interface as tracing store, we want indices
} yield indices

def querySegmentIndexForMultipleSegments(remoteFallbackLayer: RemoteFallbackLayer,
segmentIds: Seq[Long],
mag: Vec3Int,
mappingName: Option[String],
editableMappingTracingId: Option[String],
userToken: Option[String]): Fox[Seq[(Long, Seq[Vec3Int])]] =
def querySegmentIndexForMultipleSegments(
remoteFallbackLayer: RemoteFallbackLayer,
segmentIds: Seq[Long],
mag: Vec3Int,
mappingName: Option[String], // should be the baseMappingName in case of editable mappings
editableMappingTracingId: Option[String],
userToken: Option[String]): Fox[Seq[(Long, Seq[Vec3Int])]] =
for {
remoteLayerUri <- getRemoteLayerUri(remoteFallbackLayer)
result <- rpc(s"$remoteLayerUri/segmentIndex")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ class TSFullMeshService @Inject()(volumeTracingService: VolumeTracingService,
fullMeshRequest: FullMeshRequest)(implicit ec: ExecutionContext): Fox[Array[Byte]] =
for {
remoteFallbackLayer <- remoteFallbackLayerFromVolumeTracing(tracing, tracingId)
baseMappingName <- volumeTracingService.baseMappingName(tracing)
fullMeshRequestAdapted = if (tracing.mappingIsEditable.getOrElse(false))
fullMeshRequest.copy(mappingName = tracing.mappingName,
fullMeshRequest.copy(mappingName = baseMappingName,
editableMappingTracingId = Some(tracingId),
mappingType = Some("HDF5"))
else fullMeshRequest
Expand Down