-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
In Proofreading, Load Oversegmentation, Perform Merges Eagerly in Frontend #7654
Conversation
…nto magic-mapping
…nto magic-mapping
…nto magic-mapping
…nto magic-mapping
…nto magic-mapping
…tes after master merge.
moved todo-items to PR description |
Thank you for the review and the testing report 🙏
This is the only thing I still have to look into. As this shouldn't be a big deal (impact-wise), it would be cool if you could already give the branch another test?
For the record: I changed it so that these errors become soft errors (the user can retry then). there are two causes for this issue: 1) a race condition where the mapped id is not available yet (should happen very rarely; let's re-evaluate in production). 2) the dev dataset you used is cropped and the agglomerate file isn't. for that reason, the back-end serves neighbors that the front-end cannot look up. this should only be a dev-specific and therefore not critical. ideally, we would crop the agglomerate file, too.
I added a simple "Reloading segmentation layer..." message that only stays for one second (everytime the layer is reloaded). Could you check again whether this feels okay now? For me, the issue with the tool switching is now more pleasant, but it's also faster on my system. Hiding the message when the (entire? or most of the?) mapping has been loaded to the gpu is probably a bit more tricky (especially, because it's hard for me to reproduce). |
…d (i.e., super-voxels are highlightable) if not in proofreading tool
…nto magic-mapping
Luckily it was easy to fix 🎉 |
Awesome, code LGTM and no issues at all during retesting, except for one possible performance issue. I tested how fast one could go merging segments and the performance was a bit underwhelming, although merging happens entirely in the frontend. See this gif: with this console output from two different merge actions that were comparably slow: Can you reproduce this and if so do you think that could be sped up? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds support for a locally applied HDF5 mapping (instead of applying it by the back-end). This enables the user to do interactive proofreading where (a) super-voxels can be easily identified by hovering and (b) merge operations can be done without a round-trip to the back-end and (c) split operations can be done without reloading all buckets (but a roundtrip is necessary to find out which edges to delete).
The HDF5 mapping is only applied locally when the annotation has an editable mapping or is about to have one (i.e., the user is in proofreading mode). When this condition changes, the layer is reloaded automatically to swap the responsibility of mapping the segmentation between client and server.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Missing:
handleProofreadCutNeighbors
andhandleSkeletonProofreadingAction
were not yet adapted to work with frontend mappings. Look athandleProofreadMergeOrMinCut
for implementation guidance.handleProofreadCutNeighbors
handleSkeletonProofreadingAction
wkstore_adapter
currently hardcodesshouldUseDataStore
totrue
, because the backend was not yet adapted to return unmapped segment IDs from the tracing store if an editable mapping is active. If that is changed, this part of the code can be reset and brushed volume data is loaded from the tracing store correctly again. /cc @fm3T extends Map<number,number> | Map<bigint,bigint>
) to let typescript understand that the two won't be mixed, but I'm not sure whether it's possible to get it to understand that for all of the code 🤔Bugs:
jsConvertCellIdToRGBA
failsMesh loading after proofreading actions currently triggers errors, because theshould be fixed thanks to Improve segment proofreading in 3D viewport #7742getDataValue
method used to determine the agglomerate IDs after the action uses the mapping from before.[ ] double-check that mapping texture is correctly attached/detached when switching between modesdoesn't seem important to me (the opposite: unloading and re-uploading the texture when switching back and forth seems wasteful)Performance:
I did not rigorously evaluate the performance yet, but browsing through the data feels laggier than before.See In Proofreading, Load Oversegmentation, Perform Merges Eagerly in Frontend #7654 (comment).A quick profiling showed that 10% of the time is spent in the bucket'ssee comments belowgetValueSet
methods with invocations ofgetValueSets
taking up to 250 ms depending on the number of cache misses (note, union'ing the sets afterwards is quick in comparison, usually 5 ms)Writing the mapping texture is also quick in comparison ~5-10 ms(with cuckoo, updates are below 1ms typically)refreshAffectedMeshes
to web worker(didn't help much) and do it as soon as buckets arrive to distribute the workload (this is only done now when the value sets are really needed)[ ] maintain cached value union in data cubedidn't help (tested in c7bb508)Issues:
(Please delete unneeded items, merge only when none are left open)