add 'boundingBox' to NmlParser and NmlWriter #2827

rschwanhold · 2018-06-28T14:01:23Z

Mailable description of changes:

We now write the bounding box of a SkeletonTracing into the nml.
Example: <taskBoundingBox topLeftX="0" topLeftY="0" topLeftZ="0" width="512" height="512" depth="512" />
We also parse the bounding box when uploading an nml file.
- There are four possible scenarios when uploading a nml file because it is still possible to specify the bounding box explicitly in the form-value BoundinBox:
1. the bounding box is specified in the form-value but NOT in the parsed nml
  => the bounding box of the form-value is used
2. the bounding box is specified in the form-value AND in the parsed nml
  => the bounding box of the form-value is used
3. the bounding box is NOT specified in the form-value but in the parsed nml
  => the bounding box of the parsed NML is used
4. the bounding box is NEITHER specified in the form-value nor in the parsed nml
  => no bounding box is used

Steps to test:

Export the Nml of a Task via /api/annotations/:typ/:id/download
- make sure the nml has a field boundingBox if the task has a boundingBox (this is optional)
Import the Task again (Administration >> Tasks >> add Task >> 'Upload nml file')
- try setting the boundingBox in the form explicit (the form value has a higher priority)
- make sure the new Task has the specified boundingBox (from the form or the nml)

Issues:

fixes Bounding box should be persisted to / parsed from nml #2802

Ready for review

jstriebel

Please add some comments, that explain which bounding box takes precendence when & (especially) why. On which layers do we have bounding boxes now?

jstriebel · 2018-06-28T15:40:45Z

app/controllers/TaskController.scala

@@ -99,13 +99,15 @@ class TaskController @Inject() (val messagesApi: MessagesApi)
  }

  private def buildFullParams(nmlParams: NmlTaskParameters, tracing: SkeletonTracing, fileName: String, description: Option[String]) = {
+    val parsedTracingBoundingBox = tracing.boundingBox.map(b => BoundingBox(b.topLeft, b.width, b.height, b.depth))
+    val bbox = if(nmlParams.boundingBox.isDefined) nmlParams.boundingBox else parsedTracingBoundingBox


why does it make sense to give the nml bounding box precedence?

nmlParmsare the form-parameter that you specify when you are uploading an nml. There you can specify a bounding box explicitly (this is optional).
The bounding box parsedTracingBoundingBox is the bounding box which might be stored inside the nml (this is optional as well).

When you are uploading an nml and you are specifying the bounding box in the form-value explicitly then you are aware which bounding box you want. (Therefore this value is used)
When you don't specify a bounding box in the form-value then the bounding box of the parsed nml is used. (In case none of those two are specified, then the bounding box is None)

@rschwanhold: Thanks for the explanation! I thought the variables are exactly the opposite^^
Therefore I renamed them here to make this more clear.

jstriebel · 2018-06-28T15:44:14Z

app/models/annotation/nml/NmlParser.scala

@@ -56,14 +56,15 @@ object NmlParser extends LazyLogging with ProtoGeometryImplicits {
        val editPosition = parseEditPosition(parameters \ "editPosition").getOrElse(SkeletonTracingDefaults.editPosition)
        val editRotation = parseEditRotation(parameters \ "editRotation").getOrElse(SkeletonTracingDefaults.editRotation)
        val zoomLevel = parseZoomLevel(parameters \ "zoomLevel").getOrElse(SkeletonTracingDefaults.zoomLevel)
-        val userBoundingBox = parseUserBoundingBox(parameters \ "userBoundingBox")
+        val userBoundingBox = parseBoundingBox(parameters \ "userBoundingBox")
+        val boundingBox = parseBoundingBox(parameters \ "boundingBox")


is this simply None if boundingBox does not exist?

boundingBox is None if the parsed nml does not contain information about this bounding box

jstriebel · 2018-06-28T15:46:02Z

app/models/annotation/nml/NmlParser.scala


        logger.debug(s"Parsed NML file. Trees: ${trees.size}, Volumes: ${volumes.size}")

        if (volumes.size >= 1) {
          (Right(VolumeTracing(None, BoundingBox.empty, time, dataSetName, editPosition, editRotation, ElementClass.uint32, None, 0, 0, zoomLevel), volumes.head.location), description)
        } else {
-          (Left(SkeletonTracing(dataSetName, trees, time, None, activeNodeId,
+          (Left(SkeletonTracing(dataSetName, trees, time, boundingBox, activeNodeId,


Why should the boundingBox be part of the NML? I thought this belongs rather to the dataset than to a tracing?

In

webknossos/webknossos-datastore/proto/SkeletonTracing.proto

Line 63 in 50ad600

optional BoundingBox boundingBox = 4;

you can see that the bounding box is already part of a skeletonTracing.
The reason why we want to write the bounding box to the nml is that after you've downloaded the nml of a SkeletonTracing and then you are uploading it again, you dont't want to lose the bounding box. (see: https://discuss.webknossos.org/t/bounding-box-should-be-persisted-to-parsed-from-nml/966)

Ah, thanks! 👍 I wasn't aware of that layer.

daniel-wer · 2018-06-29T08:43:23Z

@rschwanhold Two things:
(1) This should probably be implemented in the frontend NML writer as well, right?
(2) I'm not sure whether we're mixing up bounding boxes here, I just want to be sure we're on the same page. From my understanding there can be a taskBoundingBox (which is what this PR is about). This taskBoundingBox (I would use this name to make it clear) will be persisted to the NML if it is there.
There also is an optional userBoundingBox (which the user may define and which is persisted in the NML as the userBoundingBox).
If both are present, both should be persisted in the NML.

Is this your understanding as well? :)

rschwanhold · 2018-06-29T09:02:12Z

@daniel-wer
(1) I forgot about this. Do you want to do this or should I try it?
(2) That is correct: we have two bounding boxes (one for the task and one for the user).
The the userBoundingBox was already persisted in the NML as you can see in

webknossos/app/models/annotation/nml/NmlParser.scala

Line 59 in 50ad600

val userBoundingBox = parseUserBoundingBox(parameters \ "userBoundingBox")

.

I think renaming boundingBox to taskBoundingBox is a good idea.
Should I also rename the variable from the SkeletonTracing to taskBoundingBox or should I only rename this in the context of the NML?

youri-k · 2018-06-29T09:12:48Z

I don't know how renaming this would affect the already stored proto data and if they need a migration

nmlParams --> nmlFormParams parsedTracingBoundingBox --> parsedNmlTracingBoundingBox

daniel-wer · 2018-06-29T09:14:28Z

(1) I'll take care of that as part of this PR :)
(2) Let's only rename it in the NML for now, as @youri-k already mentioned, I'm also not aware of the implications of changing the name in the SkeletonTracing

…s/webknossos into make-bbox-persisted

the backend code LGTM

…valid nmls (#2827)

…ssos into make-bbox-persisted

daniel-wer · 2018-06-29T10:16:07Z

I've implemented the frontend part, updated the nml snapshots and added some more tests for invalid nmls. @philippotto Please have a look at the frontend changes in c7f598a.

philippotto

Nice 🎉 Cool that you added more front-end tests, @daniel-wer 👍

philippotto · 2018-06-29T11:07:41Z

app/assets/javascripts/oxalis/model/helpers/nml_helpers.js

+    topLeftX: bb.min[0],
+    topLeftY: bb.min[1],
+    topLeftZ: bb.min[2],
+    width: bb.max[0] - bb.min[0],


Can you use convertFrontendBoundingBoxToServer from reducer_helpers.js here like this?

const [topLeft, width, height, depth] = convertFrontendBoundingBoxToServer(bb); const [topLeftX, topLeftY, topLeftZ] = topLeft; return serializeTag(name, { topLeftX, topLeftY, topLeftZ, width, height, depth });

Then, we don't duplicate the max - min logic :)

Ah, I see, you moved the code. But maybe it's a good idea, anyway, to reduce the boundingbox-related code a bit.

…ssos into make-bbox-persisted

youri-k

backend part LGTM

add 'boundingBox' to NmlParser and NmlWriter

64bc49d

fm3 assigned rschwanhold Jun 28, 2018

fm3 requested review from jstriebel and youri-k June 28, 2018 14:21

jstriebel previously requested changes Jun 28, 2018

View reviewed changes

rschwanhold and others added 2 commits June 29, 2018 11:14

renamed boundingBox to taksBoundingBox in the nml

f5b17bc

TaskController: make var names more clear

a4aecb2

nmlParams --> nmlFormParams parsedTracingBoundingBox --> parsedNmlTracingBoundingBox

Merge branch 'make-bbox-persisted' of https://github.com/scalablemind…

f0cfc03

…s/webknossos into make-bbox-persisted

daniel-wer added 3 commits June 29, 2018 12:11

add taskBoundingBox in frontend nml serializer, add more tests for in…

c7f598a

…valid nmls (#2827)

Merge branch 'make-bbox-persisted' of github.com:scalableminds/webkno…

5d0e31b

…ssos into make-bbox-persisted

Merge branch 'master' into make-bbox-persisted

180ce61

philippotto approved these changes Jun 29, 2018

View reviewed changes

daniel-wer added 2 commits June 29, 2018 13:31

apply pr feedback, use existing boundingbox conversion function (#2827)

b679298

Merge branch 'make-bbox-persisted' of github.com:scalableminds/webkno…

706fceb

…ssos into make-bbox-persisted

youri-k approved these changes Jun 29, 2018

View reviewed changes

Merge branch 'master' into make-bbox-persisted

a78b548

rschwanhold merged commit 219bcaa into master Jul 5, 2018

daniel-wer deleted the make-bbox-persisted branch July 12, 2018 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add 'boundingBox' to NmlParser and NmlWriter #2827

add 'boundingBox' to NmlParser and NmlWriter #2827

rschwanhold commented Jun 28, 2018 •

edited

Loading

jstriebel left a comment

jstriebel Jun 28, 2018

rschwanhold Jun 29, 2018

jstriebel Jun 29, 2018

jstriebel Jun 28, 2018

rschwanhold Jun 29, 2018

jstriebel Jun 28, 2018

rschwanhold Jun 29, 2018 •

edited

Loading

jstriebel Jun 29, 2018

daniel-wer commented Jun 29, 2018 •

edited

Loading

rschwanhold commented Jun 29, 2018 •

edited

Loading

youri-k commented Jun 29, 2018

daniel-wer commented Jun 29, 2018

daniel-wer commented Jun 29, 2018

philippotto left a comment

philippotto Jun 29, 2018

philippotto Jun 29, 2018

youri-k left a comment

add 'boundingBox' to NmlParser and NmlWriter #2827

add 'boundingBox' to NmlParser and NmlWriter #2827

Conversation

rschwanhold commented Jun 28, 2018 • edited Loading

Mailable description of changes:

Steps to test:

Issues:

jstriebel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rschwanhold Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-wer commented Jun 29, 2018 • edited Loading

rschwanhold commented Jun 29, 2018 • edited Loading

youri-k commented Jun 29, 2018

daniel-wer commented Jun 29, 2018

daniel-wer commented Jun 29, 2018

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youri-k left a comment

Choose a reason for hiding this comment

rschwanhold commented Jun 28, 2018 •

edited

Loading

rschwanhold Jun 29, 2018 •

edited

Loading

daniel-wer commented Jun 29, 2018 •

edited

Loading

rschwanhold commented Jun 29, 2018 •

edited

Loading