Loading precomputed meshes is allowed after brushing, but fails

[MichaelBuessemeyer]

Context

When doing a volume annotation with a fallback layer, saving it, downloading, and then re-uploading it; the option to "load precomputed meshes" is allowed for modified segments but results in the following error:

When selecting "load precomputed mesh" for a not-modified segment, this works as expected.

Expected Behavior

Either the option should not be selectable or loading the precomputed mesh should work without resulting in an error message.

Current Behavior

As described above: In the re-uploaded annotation, loading precomputed meshes is allowed and fails for modified segments. Moreover, the error message reads like the segment that is looked up is 0 which is incorrect.

Steps to Reproduce the bug:

1. Create a volume / hybrid annotation of a dataset with a fallback layer and precomputed meshes.
2. Annotate some segment X.
3. Save the tracing and download the annotation.
4. Go to the explorative annotations table and reupload the annotation.
5. In the re-uploaded annotation load a precomputed mesh for the annotated segment X. This results in an error. Loading precomputed meshes for not-annotated segments still works.

[fm3]

I investigated a bit and it turns out the reupload has nothing to do with it.

It is just generally allowed to load precomputed meshes after brushing. It will of course show outdated data for painted-over segments, and it will fail completely for new segment ids created by the brushing user.

I’m not sure what’s the best behavior here and how to build it. One option might be to disallow precomputed meshes completely in this case. Or add a warning. Or figure out which segments have been touched. I think it’s non-trivial to even figure out if brushing has occurred at all (this is why we also added the mappingIsLocked bool, which could possibly in theory be abused for this)

Any opinions? @MichaelBuessemeyer @philippotto @normanrz

[normanrz]

Adding a warning seems fine to me. Ideally, only when actually there has been any brushing (or changes to the volume layer).

[philippotto]

I also think that a warning would be appropriate. We could even show a small "alert" icon next to the "load precomputed mesh" entry when we know that brushing has occurred.

this is why we also added the mappingIsLocked bool, which could possibly in theory be abused for this

yes, there is a 1:1 relation between "a brush operation (or similar) has happened" and "the mapping is locked", right? seems reasonable to use this then. but hang on... I think, after proofreading the mapping is also locked, but precomputed meshes will work fine. so, the logic would need to be isMappingLocked && !mappingIsEditable to find out whether to show the warning.

[fm3]

I’m not a huge fan of reusing isMappingLocked for this. The logic may change in the future. Maybe we should just introduce a generic volumeBucketDataHasChanged instead. This will create some redundancy in the booleans but it would keep the naming clean and might prevent diverging logic down the line. What do you think?

[philippotto]

sure, sounds good to me :+1: