scalableminds / webknossos

Visualize, share and annotate your large 3D images online
https://webknossos.org
GNU Affero General Public License v3.0
128 stars 24 forks source link

Lock Mapping for Volume Annotations #5431

Closed fm3 closed 8 months ago

fm3 commented 3 years ago

Detailed Description

When a mapping is available for a segmentation layer, the segment IDs change. This interacts with volume annotations that use fallback segmentations.

Currently, the mapping can freely be selected at any point in time. When the user brushes, the brushed buckets are copied into the volume annotation as they currently are shown. When the user disables the mapping later, the brushed buckets stay “mapped”. When another mapping is activated, bug #5427 even hides all brushed changes and shows the fallback layer only.

We should consider how we can ensure consistency between the fallback layer data and volume annotation data. A possibility could be to pin the selected mapping in a volume annotation once you start brushing. Separating the volume annotation from the selected mapping could remove all so-far brushed changes, just like unlinking the fallback layer does.

Context

Volume annotations do make sense in the context of mappings. It is currently far too easy to create inconsistent states there, though

philippotto commented 12 months ago

@MichaelBuessemeyer I think it would be cool if you could tackle this issue. You probably need to do some digging and I'm not sure whether there's something missing in the back-end.

My idea is to implement what @fm3 suggested here:

A possibility could be to pin the selected mapping in a volume annotation once you start brushing.

We have a similar pinning mechanism when using the proofreading-tool (there an editable mapping is selected and then pinned after the first proofreading action). Maybe the same back-end functionality can be re-used.

fm3 commented 12 months ago

I still think this needs some backend changes. Currently, the locking for proofreading mappings is implicit, using the mappingIsEditable boolean of the volume tracing proto object (if I remember correctly).

Not sure if adding another bool is the right way to go here. Open for suggestions :)

philippotto commented 12 months ago

Not sure if adding another bool is the right way to go here. Open for suggestions :)

Sounds good, mappingIsPinned maybe? Or do you have alternative ideas instead of the boolean?

fm3 commented 12 months ago

I’m worried that this will create confusion in the future for proofreading annotations. What happens if mappingIsEditable is true, but mappingIsPinned is not? Semantically, all editable mappings should be pinned. However, having this precedence might lead to confusion. Having to sync the two booleans might also lead to confusion.

But I don’t really have better ideas, so maybe let’s go for it anyway.

philippotto commented 12 months ago

What happens if mappingIsEditable is true, but mappingIsPinned is not?

In my opinion, the back-end should assert that this is not the case. If mappingIsEditable is true, mappingIsPinned should be automatically true. Of course, we could use an enum-like value for the mappingState, but I'd be fine with two booleans, too.

fm3 commented 11 months ago

Ok fair enough. Should the backend set mappingIsPinned automatically on updateBucket requests? Or should we add another update action? I guess the frontend state also needs to be updated (as bucket saving does not re-fetch the tracing proto object)

philippotto commented 11 months ago

Should the backend set mappingIsPinned automatically on updateBucket requests? Or should we add another update action?

I think, another update action by the front-end is necessary, since the back-end cannot know which mapping is currently selected in the front-end, can it?

I guess the frontend state also needs to be updated (as bucket saving does not re-fetch the tracing proto object)

Yes, that's another point for the front-end being responsible, I think.

fm3 commented 11 months ago

I think, another update action by the front-end is necessary, since the back-end cannot know which mapping is currently selected in the front-end, can it?

Well the mapping name is already stored, isn’t it? I would have expected that works by the frontend sending an UpdateMappingNameAction. So if the update actions are sent in the right order, this should be ok if we do the pinning with updateBucket. However, it would be a bit implicit, so maybe another update action (or another optional bool in the existing UpdateMappingNameAction) would be nicer

philippotto commented 11 months ago

Ah, I see. I agree that an explicit update action would be nicer. I just saw that UpdateMappingNameUpdateAction already has isEditable as a parameter. An additional isPinned might make sense? Ideally, we would rename the action type altogether, but I'm not sure how easy this is with regard to backwards compatibility.

What happens if mappingIsEditable is true, but mappingIsPinned is not?

In my opinion, the back-end should assert that this is not the case. If mappingIsEditable is true, mappingIsPinned should be automatically true.

The assertion could happen on front-end and back-end then.

MichaelBuessemeyer commented 10 months ago

I do not fully understand the whole discussion above. But for me, it reads like the mapping should be pinned once a volume action is done in an annotation.

But when the pinning should be activated by the UpdateMappingNameUpdateAction action via the new isPinned the mapping would be instantly pinned as soon as it was selected and not as soon as the first brushing annotation was done.

I only see the possibility of first sending the UpdateMappingNameUpdateAction with isPinned=false and once the user brushed sending the UpdateMappingNameUpdateAction action again with isPinned=true. But this would conflict the

If mappingIsEditable is true, mappingIsPinned should be automatically true.

assertion :/

fm3 commented 10 months ago

As just discussed in person, the mappingIsEditable bool describes that the annotation is a proofreading annotation (which in the backend are called Editable Mapping Annotations). This boolean is not changed by UpdateMappingNameUpdateActions. So what you described (send one with isPinned=false first and then after the first brushing another with isPinned=true should work fine :)

Note that the backend does not yet know about this isPinned parameter/property, and we’ll have to add that there too.

MichaelBuessemeyer commented 10 months ago

As persistent Mappings are currently only supported for the HDF5 format, not JSON, testing this is kinda tricky for me. See for reference: https://github.com/scalableminds/webknossos/blob/d8a1acb98e4ff399fd97daf40a09b57a7654678b/frontend/javascripts/oxalis/model/reducers/volumetracing_reducer_helpers.ts#L159-L161

Currently, I am testing with the ROI dataset with its JSON mappings.