Disable editing of super voxel skeletons in skeleton mode

[daniel-wer]

• This PR introduces a tree type property to be able to distinguish between agglomerate skeletons and user-created trees. This property is then used to:
  • prevent (node/edge-related) modifications of agglomerate skeletons in general
  • only allow to add or delete edges of agglomerate skeletons if the proofreading tool is active
  • inform users to use the proofreading tool if they are trying to modify agglomerate skeletons while the proofreading tool is not active

TODO:

• Backend ToDos, see Disable editing of super voxel skeletons in skeleton mode #7086 (comment)
• Include tree types in NML
• Should agglomerate skeletons be visually distinguished from user-created skeletons somehow?
• [ ] [Optional] Allow to convert agglomerate skeletons to freely modifiable trees

URL of deployed dev instance (used for testing):

• https://proofreadingskeletonmodification.webknossos.xyz

Steps to test:

• Create an annotation on a dataset with an agglomerate view (e.g. test-agglomerate-file)
• Create a skeleton by clicking some nodes
• Load an agglomerate skeleton
  • Try to delete a node or add new nodes -> Nothing should happen and an error toast should be shown
  • Try to delete or add an edge -> Nothing should happen and an error toast hinting at the proofreading tool should be shown
• Switch to the proofreading tool
  • Try to delete a node or add new nodes -> Nothing should happen and an error toast should be shown
  • Try to delete or add an edge -> Should work as before
• Reload the annotation. The tree types should be restored.
• Export and import an NML using the backend and frontend functionality. Non-default tree types should, and default tree types can, be serialized to NML.

Issues:

• fixes Disable editing of super voxel skeletons in skeleton mode #6935

---

(Please delete unneeded items, merge only when none are left open)

• Updated changelog
• Updated documentation if applicable
• Adapted wk-libs python client if relevant API parts change
• Removed dev-only changes like prints and application.conf edits
• Considered common edge cases
• Needs datastore update after deployment

[daniel-wer]

@philippotto It would be great to get some feedback from you regarding the general approach. If you agree that this is how we should do it, we need to make sure that the type property is persisted across reloads and also need to discuss the other TODOs. Don't be afraid by the number of changed lines - most of that stems from me typing three test files. The actual changes are much smaller :)

[philippotto]

Awesome 💯 I think, this approach is the right one 👍 I looked through the changes and only noticed smaller stuff mostly. What is your opinion about the trees of the connectome tab? We should probably port them to the new approach, too? But maybe not in this PR?

Regarding the todos:

Persist tree type in backend

👍

Should it be included in NMLs?

I think, this would be cool and probably not too much effort. Alternatively, one could ignore non-default trees during export..

Should agglomerate skeletons be visually distinguished from user-created skeletons somehow?

I think, this would be very useful. A special icon (plus tooltip) might be enough. Alternatively, the agglomerate skeletons could be rendered in the segments-tab under their respective segments. However, that's probably a bit more effort. Not sure whether it's worth it?

Allow to convert agglomerate skeletons to freely modifiable trees

Sounds like a nice-to-have to me.

Don't be afraid by the number of changed lines - most of that stems from me typing three test files. The actual changes are much smaller :)

Kudos for doing that 🎩 🎉

[daniel-wer]

@fm3 As discussed yesterday, this PR introduces a type property for trees which currently can either be DEFAULT (which is the default for existing trees) or AGGLOMERATE. It's likely that this will be extended in the future, the next candidate could be CONNECTOME or so (for the trees of the connectome viewer tab). It's likely that the tree type will be changeable in the future (to convert an agglomerate skeleton to a user-modifiable skeleton), though this won't be added in this PR.

There are few small backend-related changes needed for this PR:

• Extend the createTree and updateTree update actions to include the tree type (I have not done that in the frontend, yet to avoid errors)
• Persist the tree type in the backend and include it when serving skeleton tracings
• Set the tree type to AGGLOMERATE when serving agglomerate skeletons
• Serialize and parse it to/from NML files
• Anything else that comes to your mind?

@philippotto If you have suggestions regarding the naming of the property or the enum values, this would be a good time to raise them :)

[daniel-wer]

Do any of you know whether the libs need to be updated to cope with the tree type property or whether they ignore unknown properties?

[philippotto]

@philippotto If you have suggestions regarding the naming of the property or the enum values, this would be a good time to raise them :)

I'm fine with the current suggestions :) However, I'm not sure whether the word type could cause problems, because it might be reserved in some contexts? I remember that we also have typ at some places. Alternatively, I'd suggest role, but don't have strong opinions here.

Do any of you know whether the libs need to be updated to cope with the tree type property or whether they ignore unknown properties?

I didn't test it, but this code here strongly looks like it wouldn't care:
https://github.com/scalableminds/webknossos-libs/blob/82e1e0ec4aafcf2e9240d6b80ac3b506070a14ab/webknossos/webknossos/_nml/tree.py#L47

I think I also remember that we added other properties in the past and didn't take any precautions against other parsers (e.g., matlab ones).

[fm3]

@daniel-wer I added the type attribute in the skeleton protobuf, update actions and nml. I did not test it yet, as the front-end update actions are not yet adapted. please have a look :) Note that the front-end NML export/import should probably also handle the new attribute. Also note that it is an optional attribute for backwards compatibility. It is also usually not set (and thus also not serialized to NML). The code should be able to handle that.

Please ping me if you need anything else!

[daniel-wer]

Thank you @fm3!

Note that the front-end NML export/import should probably also handle the new attribute. Also note that it is an optional attribute for backwards compatibility. It is also usually not set (and thus also not serialized to NML). The code should be able to handle that.

All of that should already be implemented :)

I added the type to the update actions and everything seems to work well from my testing.

I didn't test it, but this code here strongly looks like it wouldn't care:
https://github.com/scalableminds/webknossos-libs/blob/82e1e0ec4aafcf2e9240d6b80ac3b506070a14ab/webknossos/webknossos/_nml/tree.py#L47

I think I also remember that we added other properties in the past and didn't take any precautions against other parsers (e.g., matlab ones).

@philippotto Great, thanks for looking it up 🙏

I'm fine with the current suggestions :) However, I'm not sure whether the word type could cause problems, because it might be reserved in some contexts? I remember that we also have typ at some places. Alternatively, I'd suggest role, but don't have strong opinions here.

It doesn't seem to cause any issues in javascript or scala. Since Python doesn't have object destructuring, it shouldn't pose any issue there, too. Most of the time, it's probably a good idea to name resulting variables treeType anyway.

[philippotto]

Awesome, works really great 👍 Only commented nitpicks. Also, I noticed one tangential: The clear-byproducts button in the toolbar has this tooltip: Clear auxiliary skeletons and meshes that were loaded while proofreading segments. However, this button doesn't remove any skeletons. Not sure when this diverged from the actual behavior, but could you remove "skeletons and " from that string?