Make Largest Segment ID Optional

[fm3]

Makes the largestSegmentId for segmentation layers optional. Persists the largestSegmentId for volume annotation layers in the NML

TODO Backend

• [x] make largestSegmentId optional in case classes
• [x] adapt datasource validation
• [x] create unset value for volumeTracings (cannot use None here because of proto backwards compatibility)
• [x] volumeTracing largestSegmentId: persist to and read from NML
• [x] volumeTracing largestSegmentId: make optional

TODO Frontend

• [X] when creating a new segment id, check if the volumeTracing’s largestSegmentId is null. If so, tell the user about it and provide a way to input a largest segment id
• [x] Relax the json validation in the dataset edit view
• [x] Adapt warning message and validation in add-remote-zarr-dataset view

TODO python client

• [x] make sure that existing requests don’t break if the segmentation layers do not have a largestSegmentId field https://github.com/scalableminds/webknossos-libs/pull/786

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• dataset settings page
  • edit a dataset with a segmentation and set the "largest segment id" field to empty
  • switch to the "advanced" menu and doublecheck that the largest segment id property is either set to null or does not exist
  • add the property there and set it to 10
  • switch back to simple
  • switch back to advanced and remove the property again
  • switch back to simple
  • the field should be empty
  • save
• create an annotation for that dataset (fallback layer should be the segmentation layer without the largest segment id)
  • switch to brush
  • try to brush -> the active segment id should be zero which shouldn't do anything and a toast should warn you about this
  • try to create a new segment id via "c" or the toolbar button --> a toast should appear
  • click the button in the toast and a modal should open which allows to input a largest segment id
  • do so and click ok
  • now, the "c" shortcut should work
• create an annotation without fallback layer --> brushing should just work

Issues:

• fixes #6039
• fixes #6390

---

• [ ] Updated (unreleased) changelog
• [ ] Updated documentation if applicable
• [x] Adapted wk-libs python client if relevant API parts change
• [x] Needs datastore update after deployment
• [x] Ready for review

[fm3]

@philippotto Would you be interested in implementing the front-end todos here?

@jstriebel Would you be interested in adapting the python client? I think the important part is that the segmentation layers in the info request may no longer contain the largestSegmentId, which might break existing requests? If the typing is that strict in the front-end.

[jstriebel]

@jstriebel Would you be interested in adapting the python client? I think the important part is that the segmentation layers in the info request may no longer contain the largestSegmentId, which might break existing requests? If the typing is that strict in the front-end.

Yep: https://github.com/scalableminds/webknossos-libs/pull/786

[philippotto]

@philippotto Would you be interested in implementing the front-end todos here?

Yes, I think, I can do that. Not sure when exactly I'll get to it, though :see_no_evil:

[fm3]

@philippotto I changed the proto file to allow None for largestSegmentIds. Note that in the update actions they are still not nullable. When I tried to create an empty volume annotation (no fallback layer), I got the please-enter-id prompt. I believe the backend would have set the Id to Some(0). Do you know if the frontend interprets that as null? Otherwise there might still be something wrong in the backend.

[philippotto]

@philippotto I changed the proto file to allow None for largestSegmentIds. Note that in the update actions they are still not nullable.

Can we make the property in the update action nullable? Or does something speak against it?

When I tried to create an empty volume annotation (no fallback layer), I got the please-enter-id prompt. I believe the backend would have set the Id to Some(0). Do you know if the frontend interprets that as null? Otherwise there might still be something wrong in the backend.

Yes, sorry, there was an error in the front-end when I introduced a temporary logic to handle -1 as a value.

[fm3]

I was still assuming that volume annotating was essentially impossible while no largestSegmentId exists. But from what you mentioned in the meeting, it sounds like you want to only forbid those actions that reaally need it. So I also made it optional in the update action now, to support that.

[philippotto]

I was still assuming that volume annotating was essentially impossible while no largestSegmentId exists. But from what you mentioned in the meeting, it sounds like you want to only forbid those actions that reaally need it. So I also made it optional in the update action now, to support that.

Yes, exactly :+1: I had this idea on Friday during implementing the changes. I think, it makes sense, since a user might want to correct existing segments for which a new segment is not even needed. Also, even if we disallowed any annotating features, there could still be update actions such as updating the position or segments. Disallowing all of that would be a bit drastic in my opinion.

I'll let you know once the frontend is polished. Then, you could also have a look to see how this approach feels.

[philippotto]

I think the front-end is ready for review. @fm3 Do you think the back-end is ready, too? Feel free to test the frontend behavior now :) If you agree with the approach, I'd ask for a review.

[fm3]

Yes, I’d say the backend is also ready for review. I tested the behavior and it makes sense to me.

However, I’d suggest that we explain to the user a bit more what is happening, or guide them towards a safe path. I guess the standard use case is that the user will create an annotation and start brushing. The popup currently only says The segment id is 0, please change it. »Cannot brush with segment id 0. Please change the segment id by [etc] or create a new unused id in the toolbar.« (or something) – where the user will then also be asked to set the largest segment id. It might even be nice to add a link to the dataset edit view, if the user has write access to the dataset, so that they can then set the largest segment id for the on-disk layer. I hope this makes sense, what do you think?

[philippotto]

However, I’d suggest that we explain to the user a bit more what is happening, or guide them towards a safe path. I guess the standard use case is that the user will create an annotation and start brushing. The popup currently only says The segment id is 0, please change it. »Cannot brush with segment id 0. Please change the segment id by [etc] or create a new unused id in the toolbar.« (or something) – where the user will then also be asked to set the largest segment id. It might even be nice to add a link to the dataset edit view, if the user has write access to the dataset, so that they can then set the largest segment id for the on-disk layer. I hope this makes sense, what do you think?

I extended the docs for the largest segment ID and also linked the "dataset edit view" in the new modal. Other than that, I think, that the problem is explained sufficiently in the modal, no? The two new toasts are not too detailed, but I don't think that they should be much longer.

Do you think this is alright?

[fm3]

@jstriebel Thanks for having a look! This is why reviews are important, this code was really quite cryptic :D I added some comments now, hopefully answering your questions. I also merged the precedence methods from AnnotationService and AnnotationIOController into one, and while I was there changed the artificially Fox-y method to synchronous. adaptPropertiesToFallbackLayer changes the uploaded VolumeTracing’s properties, depending on the fallback layer. If there is one, its properties are used, if there is none, the default properties for no-fallback-layer tracings are inserted. I agree that the name does not fully reflect that, but I don’t really have a better idea that doesn’t get ridiculously long. Please have another look, I hope the changes are explained now :)