Enforce TaskType Resolution Restrictions for Multi-Res Volume Tasks #4891
Conversation
|
Awesome 👍
Just for my understanding: will it be slower than the previous downsample-on-upload approach or "equally slow"? |
|
about equal, I’d think. not sure if fetching the data from fossildb vs unpacking the data from the zip has much influence, but i’d guess the bulk time comes from the actual downsampling computation. I just wrote that note to indicate that this should probably be communicated to the user with a spinner. for my sample volume annotation it took 10s, but it may also be several minutes for larger ones. in the future we may of course also consider performance measurements and think about speedup :) |
|
Ok, cool, thanks for the info! If the request takes longer than a minute, we might run into request timeouts again due to the browser, though 🙈 Could you add a |
…s/webknossos into multires-volumes-restricted
…s/webknossos into multires-volumes-restricted
The reload button next to the active-resolution-indicator only appears for explorative annotations with volume data. It opens a modal which explains the action and its consequences. While the operation is running, the modal blocks further usages of webKnossos. After the operation has completed, the complete page is reloaded.
…s/webknossos into multires-volumes-restricted
I've implemented this now. Still a bit hesitant about the details, but might work for now. The entire page will reload which should be sufficient.
This is currently not implemented. I'm wondering if there are use cases where one wants a "fresh" downsampled version of the tracing. Our ad-hoc downsampling approach is a bit of a heuristic, which is why I could imagine that downsampling from scratch could be useful from time to time. What do you think, @fm3?
An argument against showing the downsample-option all the time is probably that it could confuse the user, as to why this button is there in the first place. I also thought about moving it into the layer-sidebar (but that's already quite crammed) or in the dropdown next to the save button (semantically, this seems to be the worst option). Would be happy to hear your opinion on that :) |
|
Good question! I agree that in some cases this strictly “mode”-based downsampling may be desired, overwriting the heuristic one. However, I believe it will be a lot more common to just have an annotation with an incomplete resolution pyramid. The explanation in your screenshot is good but it targets mostly the overwriting case (“re-create”). I think if we really want to support both use cases, we need two differing explanations, rather than one that tries to explain both. So if the resolution pyramid is already present, show a text similar to your draft, but if not, show a simpler version, that does not mention re-creation. Also both explanations may benefit from actual values for the resolutions present and those about to be created (this information should be present in the front-end, right?). However, my assumption is that it would be fine to actually support only the one use case, and hide the button if the resolution pyramid is already complete. This will reduce user confusion. If the difference between the mode downsampling and the heuristic downsampling is so significant that we need to explain it to the users and offer both, maybe the heuristic is too bad to use ever (but I think it’s ok). Let me know what you think. On the button location, I think I’d have looked for it in the layer settings, but info tab is fine too. (Maybe even both? Not sure what the UI guidelines on redundant buttons are) |
...ore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala
Show resolved
Hide resolved
...ore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala
Outdated
Show resolved
Hide resolved
fm3
changed the title
…was forbidden by the task type
…s/webknossos into multires-volumes-restricted
Co-authored-by: Philipp Otto <[email protected]>
…s/webknossos into multires-volumes-restricted
...pp/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingDownsampling.scala
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/controller/combinations/skeletontracing_plane_controller.js
Outdated
Show resolved
Hide resolved
| "ZOOM_BY_DELTA", | ||
| "SET_ZOOM_STEP", | ||
| "SET_STORED_LAYOUTS", | ||
| "SET_TOOL", |
There was a problem hiding this comment.
Does the tool have any effect on this saga?
Also, if I remember correctly, the zoom step could also be influenced by the SET_INPUT_CATCHER_RECT and SET_INPUT_CATCHER_RECTS actions.
There was a problem hiding this comment.
Good catch regarding the SET_TOOL action 👍 This is a leftover from a version where the saga switched the tool (instead of the volume tools view). I'll remove it.
Also, if I remember correctly, the zoom step could also be influenced by the SET_INPUT_CATCHER_RECT and SET_INPUT_CATCHER_RECTS actions.
I just verified that SET_STORED_LAYOUTS is called when resizing the viewports and when toggling the fullscreen mode. So, the other actions should not be necessary to listen to. Let me know if other cases come to your mind where it could be important, too.
Probably, the store definition for the input_catcher_rects and the layouts could use some clean up, as their semantics overlap here. However, I'm hesitant to touch that code 🙈
There was a problem hiding this comment.
I just verified that SET_STORED_LAYOUTS is called when resizing the viewports and when toggling the fullscreen mode. So, the other actions should not be necessary to listen to. Let me know if other cases come to your mind where it could be important, too.
Ah okay, cool, thanks for checking 👍
| This annotation does not have volume annotation data in all resolutions. Consequently, | ||
| annotation data cannot be rendered at all zoom values. By clicking "Downsample", | ||
| webKnossos will use the best resolution of the volume data to create all dependent | ||
| resolutions. |
There was a problem hiding this comment.
This may overwrite existing resolution, for example, if mags 1 and 2 exist, mag 2 is overwritten, right? If so, this should be noted here or maybe better, in the next paragraph: "The following resolutions will be overwritten ...".
There was a problem hiding this comment.
If my previous comment holds true, is there a technical reason why the best resolution is used for downsampling, possibly overwriting existing resolutions, instead of downsampling from the "worst" existing resolution?
There was a problem hiding this comment.
Technically, you are correct, but conceptionally the mags should always be in sync. Thus, discarding downsampled mags and re-creating them should not do any harm, which is why I wouldn't want to unsettle/confuse the users with that information. Do you agree?
There was a problem hiding this comment.
Technically, you are correct, but conceptionally the mags should always be in sync.
Yes if that's the case, I agree :)
|
|
||
| <p style={{ fontWeight: "bold" }}> | ||
| Note that this action might take a few minutes. Afterwards, the annotation is reloaded. | ||
| Also, the version history of the volume data will be reset. |
There was a problem hiding this comment.
Out of curiosity, why does the history need to be reset?
There was a problem hiding this comment.
I think, the entire tracing object within the annotation is swapped out with a new one. Since history is only persisted for a tracing, such swap-outs cannot be recorded. The reason for swapping out the entire volume tracing is probably, since the new tracing needs to have a different resolution set?
Correct me if I'm wrong, @fm3!
Obviously, it would be cool if we could keep the history. @fm3 Would this be feasible for a later iteration?
There was a problem hiding this comment.
Yes, that’s correct, the downsample call is a variant of duplicate. This was the easiest way to implement this and ensure no version conflicts can occur etc. However, it does not have to be done like this, a future iteration could probably implement the downsampling as writing new version in an existing volume tracing.
The backend stdout logging also includes both the old and the new tracing id along with the annotation id if this happens, so if a user really needs to undo this, we can do it for them in the db.
frontend/javascripts/oxalis/view/settings/dataset_settings_view.js
Outdated
Show resolved
Hide resolved
…s/webknossos into multires-volumes-restricted
Co-authored-by: Daniel <[email protected]>
…s/webknossos into multires-volumes-restricted
|
* keep fixed zoom during td rotation to planes * refactor 3d rotation method to keep the zoom and simplify it * fix position and up vector not getting interpolated correctly * add changelog entry * Apply suggestions from code review Co-authored-by: Daniel <[email protected]> * remove zReductionFactor * backend: support anisotropic resolutions in volume bucket keys * fix 3d rotation preset * remove useless check for valid values * remove reassignment of width and height * do volume annotation in all resolutions * Update frontend/javascripts/oxalis/controller/camera_controller.js Co-authored-by: Philipp Otto <[email protected]> * made code pretty * reduce max brush size * disallow trace tool in higher resolutions * enable brushing in high again * do not block resolution loading * annotate z many slices at once when z resolution is > 1 * fixing a few tests * hopefully fixed all tests * Add info about annotating multiple slices * limit area that gets autofilled * [WIP] lazy downscaling of volume buckets * lookup-based live downsampling * [WIP] downsample volume tracings on upload * [WIP] downscale during upload * fix offsets, switch to mode * determine which mags to downsample * adapt nml unit test suite * fix backend dummy tracing * move function to get number of slices to accessor * WIP: applying flood fill to all resolutions * [WIP] track mags in volumetracing object * fix calculating error during downsampling * fix flood fill for all source resolutions * fixed flood fill for all resolutions * handle differing resolutions during volume merging + upload * add up and downsampling of LabeledVoxelsMap * add unlinkFallback route * changelog * cleanup backend code * fixed up and downsampling and added tests * added method to apply a voxel map * fixed upsampling and added regression test * add comments to sampling methods * make copylayer work in multi resolutions * while upsampling annotate mutliple slices * Add todo about refactoring labeling voxels * adjusted voxel labeling method to label in all resolutions the segmentation layer has * added some comments with todos * Update frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.js * always label in all resolutions when using api; show stack-icon when multi-slice-annotating * more comments * replace old unlinkFallbackLayer with new API for that (keep old UpdateAction for backwards compatibility) * fix wrong parameter format * refactor resolution handling to deal with sparse resolutions better (WIP/1) * undo temporary new_resolutions change * continue refactoring resolution handling to deal with sparse resolutions better (WIP/2) --> brush, paint-bucket and copy-segmentation tool seem to work * fix picking segment color when being in a not-existing segmentation mag * ensure that prefetch saga, bucket picker and pull queue don't try to load buckets for not existing mags * ensure legacy getResolutions method provides dense resolution set; clean up * fix initialization and volume annotation sampling spec * fix CI * update snapshots * tune warning text * make merger mode work in higher resolutions * show a status indicator in the viewports if a layer cannot be rendered due to the zoom step * reduce tooltip rerenderings by making styles constant * fix enabled 'Render Missing Data Black' for missing mags * add issue links to todo comments * implement some feedback * Apply suggestions from code review Co-authored-by: Daniel <[email protected]> * more PR feedback * also implement getIndexOrClosestHigherIndex for ResolutionInfo and use where appropriate * return true in isVolumeTraceToolDisallowed if no volume tracing exists * fix front-end merge conflicts * reduce usages of getResolutionByIndexWithFallback and make that function correct (remove isotropic fallback in favor of failing for unclear cases * simplify is2DVoxelInsideBucket method in Bucket * Apply suggestions from code review Co-authored-by: Daniel <[email protected]> * integrate more PR feedback; resolve merge conflict in changelog; rename missing to unrenderable etc * refactor applyLabeledVoxelMapToAllMissingResolutions * further refactoring of applyLabeledVoxelMapToAllMissingResolutions * remove unused import * fix flow after upgrade * improve comment for LabeledVoxelsMap * fix uniform definion * fix getDataValue for missing resolution * Improve performance of volume annotation tools (#4848) * tmp: use LabeledVoxelsMap for brushing and TypedArray for brushing map (first measurements yield 20x better performance) * restore all of the brush functionality; create LabeledVoxelMap in current mag; further performance optimizations * clean up VoxelIterator a bit * more clean up * clean up and fix CI * further clean up * remove todo * use unzoomed centroid when updating direction * fix contour drawing in mag > 1 * fix flow * fix circle-drawing for anisotropic mags * update changelog * rename VoxelIterator to VoxelBuffer2D * optimize draw circle method by comparing squared dist with squared radius * rename variables and change comment for bx and by * remove obsolete comment * add another comment to fillCircle call * ensure that rendered volume magnification is also the mag in which is annotated - also ensure that the volume toolbar is disabled properly when the segmentation cannot be rendered for some reason * Update frontend/javascripts/oxalis/model/sagas/volumetracing_saga.js Co-authored-by: Daniel <[email protected]> * fix issues from merging master * fix upload/download * fix issues after merge * disable continuous drawing for now since its performance needs to be adapted to the multi-resolution feature * fix linting * adapt headline in import modal * change tracing to annotation * change magnification to resolution in user-facing strings and some internal ones * make resolution-warning less verbose * try to fix e.trigger is not a function * fix linting * fix bug in applyLabeledVoxelMap * fix flow * show fallback-not-included warning only when there is fallback data * fix anisotropic bucket adresses * treat empty resolution list as no resolution list * change Boolean() to != null * fix flow in model initialization * ensure volume layer is saved before reloading; fixes #4857 * format * disable copy-segmentation feature for higher mags due to performance * fix some merge-related problems * fix styling of multi-slice-icon * throw an exception if the resolution could not be looked up instead of defaulting to 0,0,0 * look up volume bucket resolution by zoom step ** 2 rather than index * fix that toolbar rerendered on every state change * improve handling of volume-is-disabled case in toolbar * replace exception by fox.failure if volume resolution lookup fails * show data type and resolutions of layer in tooltip next to layer name (therefore, the dtype tag is removed) * fix syntax error * Enforce TaskType Resolution Restrictions for Multi-Res Volume Tasks (#4891) * [WIP] enforce task type magnification settings in tasks * pass allowedMagnifications to rpc methods * pass magnifications as query string * respect resolution restrictions for volume tracings. * add option to downsample existing volume tracing * remove outdated assertion, update resolution list after downsampling * sleep 10s * remove sleep and bump dev-proxy-timeout to 5 min * fix task creation with resolution restrictions * use inclusive check * when uploading zips with differing resolution sets, fail * remove unused import * refresh snapshots (tracings contain organization name) * write header.wkw to every mag in volume download, fix anisotropic directory names * add resolution-samenss assertion in volume dnd import. fix flow types * fix restriction passing in initial data * Add button to trigger downsampling of volume annotation. The reload button next to the active-resolution-indicator only appears for explorative annotations with volume data. It opens a modal which explains the action and its consequences. While the operation is running, the modal blocks further usages of webKnossos. After the operation has completed, the complete page is reloaded. * fix parameter passing in duplicate rpc * avoid undefined behavior of bucket iterator by checking hasNext. do not relabel when “merging” only one volume * make tasktype resolution restrictions immutable. sort resolution list * update migration guide * remove debug output * improve initiate-volume-downsampling modal and move its button to the layer settings * show toast when mag-restriction is violated instead of prohibiting the zoom change in the first place * don't allow editing mag-restrictions in already created tasktype * remove some more debug output, unify variable names * forbid creating new nodes or using volume tools when the current mag was forbidden by the task type * clean up * Update MIGRATIONS.unreleased.md Co-authored-by: Philipp Otto <[email protected]> * fix styling of button-link * use ListBuffer instead of HashSet as per pr feedback * integrate some feedback * Apply suggestions from code review Co-authored-by: Daniel <[email protected]> * remove unnecessary SET_TOOL dependence * do map ids if initialLargestSegmentId != 0 Co-authored-by: Philipp Otto <[email protected]> Co-authored-by: Philipp Otto <[email protected]> Co-authored-by: Daniel <[email protected]> Co-authored-by: Michael Büßemeyer <[email protected]> Co-authored-by: MichaelBuessemeyer <[email protected]> Co-authored-by: Daniel <[email protected]> Co-authored-by: Philipp Otto <[email protected]> Co-authored-by: Youri K <[email protected]> Co-authored-by: Philipp Otto <[email protected]>
Changes for the larger PR #4755
PATCH /annotations/:typ/:id/downsample(explorative only) takes lowest present mag (e.g. mag1) and creates pyramid on top of that one, returns annotation object with new volume tracing id. may be slow.TODO
initialDatainitializeWithDataMultiple) → assert they are equal.→ they should not
explaining toast)
URL of deployed dev instance (used for testing):
Steps to test:
→ Could not test this due to uploading multiple volumes is broken #4903 deferred to later PR
→ Could not test this due to uploading multiple volumes is broken #4903 deferred to later PR