Lock mapping once the volume annotation was changed by the user #7549
Conversation
|
@fm3 Here is the PR for the Pin Mapping functionality. The frontend part should work with the ROI dataset and its mappings but still needs some clean up after the backend part is done. Would you please implement the backend part? 🙏 |
|
@MichaelBuessemeyer I added the backend part, seems to work now :) Please ping me again if you need anything else |
|
@daniel-wer could you please have a look at the frontend part and @frcroth could you please review the backend part? 🙏 |
|
@fm3 How do you think we should handle |
|
Yes, probably. Did you test if that works as expected? I haven’t worked with JSON mapings in a while |
| @@ -327,11 +327,13 @@ class VolumeTracingController @Inject()( | |||
| for { | |||
| tracing <- tracingService.find(tracingId) | |||
| tracingMappingName <- tracing.mappingName ?~> "annotation.noMappingSet" | |||
| _ <- bool2Fox(tracing.mappingIsPinned.getOrElse(false)) ?~> "annotation.mappingIsPinned" | |||
There was a problem hiding this comment.
Why is the default here false, which leads to a failure, when before it would work? Maybe I don't get this line, but the error "annotation.mappingIsPinned" suggests that a problem would occur be if the mapping were pinned, even though this line requires the mapping to be pinned.
There was a problem hiding this comment.
Excellent catch, I forgot a "not" here. I changed it now and tested that the behavior is as expected.
@MichaelBuessemeyer could you disable the whole proofreading tool in the frontend once the mapping is pinned (unless it is already a proofreading annotation i.e. mappingIsEditable==true)? Proofreading actions will fail after the first brush stoke as well.
|
General question: should we offer a way to undo this? If the user brushes without knowing that this will disable some stuff, they might want to undo that to restore the prior state. Currently, the mapping would stay pinned. Do you think we should support this? I guess that the restore older version view would do the trick. |
Yeah, I think we should support this. Maybe we should make the whole process of pinning the mapping more transparent? Once the user starts a volume annotation, we could open up a modal to inform the user, that the mapping will be pinned from now on, in case the user accepts the brushing action. @philippotto and maybe @fm3: On second thought I think I skipped rare cases where the user is able to do an volume annotation without the mapping getting pinned via the copy to next layer functionality: e.g.
I think there are more possible cases that do a volume annotation but are not caught by the current mechanism implemented in volumetracing_saga.tsx. |
Undoing is not necessary imo, but asking the user if they are okay with the pinning sounds good.
Yes, ideally, these cases would all be caught. Otherwise, this might produce hard-to-debug errors. |
| const { id: layerName, mappingName, mappingIsPinned } = volumeTracing; | ||
|
|
||
| if (!(layerName in stateByLayer)) { | ||
| stateByLayer[layerName] = {}; | ||
| } | ||
| if (stateByLayer[layerName].mappingInfo == null) { | ||
| stateByLayer[layerName].mappingInfo = { | ||
| mappingName, | ||
| mappingType: "HDF5", | ||
| }; | ||
| if (stateByLayer[layerName].mappingInfo == null || mappingIsPinned) { | ||
| // A pinned mapping always takes precedence over the URL configuration. | ||
| if (mappingName == null) { | ||
| delete stateByLayer[layerName].mappingInfo; | ||
| } else { | ||
| stateByLayer[layerName].mappingInfo = { | ||
| mappingName, | ||
| mappingType: "HDF5", | ||
| }; | ||
| } |
There was a problem hiding this comment.
When canceling a brush action in the confirmation modal, the saga crashes (same for other tools, although sometimes only an error on the console and a toast is shown)
Thanks for catching this. I initially thought that a change in the volumetracing_reducer caught this, but it seems I was wrong. This is the code location that determines whether to load the mapping based on what the backend saved annotation says vs. what the URL state says. I modified the code in such a way, that in case the mapping is pinned, the settings from the backend should overwrite the URL settings.
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
Outdated
Show resolved
Hide resolved
| try { | ||
| return yield* call(askUserForPinningActiveMapping, volumeTracing, activeMappingByLayer); | ||
| } catch (error: any) { | ||
| return error as EnsureMappingIsPinnedReturnType; | ||
| } |
There was a problem hiding this comment.
When canceling a brush action in the confirmation modal, the saga crashes (same for other tools, although sometimes only an error on the console and a toast is shown)
Thanks for finding this. I simply forgot to catch the rejected promise properly here. It should be fixed now :)
philippotto
changed the title
There was a problem hiding this comment.
Thank you for incorporating the feedback! Code mostly LGTM, but I still encountered some issues during testing:
- When aborting a flood fill in the lock confirmation modal, the flood fill is performed anyways (and the mapping is not locked).
- When locking a mapping by brushing with an activated mapping and then reloading the page, the mapping no longer seems to be locked. Is this a regression due to the renaming?
- Might be because of the previous point, but this is still not resolved for me: "When activating a JSON mapping and copying the sharing link, then switching to an agglomerate view mapping and brushing (thereby pinning the mapping) and then opening the previously copied sharing link, the JSON mapping gets activated."
frontend/javascripts/oxalis/model/actions/volumetracing_actions.ts
Outdated
Show resolved
Hide resolved
|
Hi @daniel-wer, thank you for your feedback and thorough testing. I hope I found every occurrence of "pin / pinned" and renamed it accordingly.
Did you maybe not wait long enough to reach a save state / for all update actions being sent to the server to persist in the locked state? For me, the locked state is working after clicking the save annotation button, waiting, and then reloading. => Maybe I should enforce that the annotation reaches a saved state after the mapping is locked?
Same here as above. Or maybe the dev deployment was simply not in a good state due to the renaming 🤔? What do you think about this suggestion? |
There was a problem hiding this comment.
Thanks for pushing this through, works beautifully 🎉
I'm pretty sure I made sure the annotation was saved when previously testing and even removed/reinstalled the dev deployment, but who knows what happened there. Now it worked like a charm.
I don't think it's needed to ensure that a saved state is reached after a mapping was locked. The mapping lock update action will be the first in the save queue and if that one doesn't reach the server, the bucket modification actions following afterwards won't reach the server either, so the state will remain consistent if users decide not to save.
|
@daniel-wer Am I right to assume that this PR can be merged? Or is there something missing? :) |
|
Yes, it can be merged. |
URL of deployed dev instance (used for testing):
Steps to test:
test-agglomerate-fileTODOs
Issues:
(Please delete unneeded items, merge only when none are left open)