Skip to content
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

Fix volume bugs, add tests for volume brushing & undo #5728

Closed
wants to merge 23 commits into from

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Sep 13, 2021

  • Fix 2 bugs:
    • Brushing twice with pre-existing and unfetched data on back-end (either via fallback or because this was already annotated) and performing Undo once will not correctly merge the back-end data. The fetch-promise necessary for that logic was only attached to the first brush action. Also see the new test.
    • Dirty buckets were never set to not-dirty after saving. This could lead to forceful bucket evictions when annotating over 5000 buckets.
  • Add tests for the above bugs (without the fixes, the tests failed, now they succeed)
  • Misc:
    • Make test output less noisy
    • emit AsyncTaskQueue errors on console
    • rename bucket methods to markAs{Pulled, Pushed}
    • Fix babel watch and ava debug
    • Don't import volumetracing_reducer.spec.js in test (this would execute the tests as soon as something was imported from that module)
    • Fix ava watcher (uses chokidar) which choked (oh, the irony) on my binaryData folder even though that shouldn't even be watched 🤷
    • Remove some dead code (mostly some old events, such as bucketLoaded, which are already listened to by the TextureBucketManager)

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • See new CI tests which should cover a bit.
  • I also tested ortho and flight mode to check whether data loading works as before

@philippotto philippotto self-assigned this Sep 13, 2021
@philippotto philippotto changed the title [WIP] Remove some dead code Fix volume bugs, add tests for volume brushing & undo Sep 14, 2021
@@ -238,16 +238,3 @@ test("Garbage Collection should not collect buckets with shouldCollect() == fals
const addresses = cube.buckets.map(b => b.zoomedAddress);
t.deepEqual(addresses, [[0, 0, 0, 0], [3, 3, 3, 0], [2, 2, 2, 0]]);
});

test("Garbage Collection should not throw an exception if no bucket is collectable", t => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this test, since I find my new test (which provokes the error) more important than this one. For keeping both, I didn't have a good idea, and I also think that it's not super valuable.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is awesome! Thank you for deep-diving into this and spending the time to remove left-over code as well as kick-starting the real:tm: volume tracing integration testing. The work in this PR will serve as the base for further volume tracing testing in the future, for example, the up/downsampling code, redo and the non-brushing modes.

As I wrote I feel like snapshot testing could make this even more solid. Would be interested in what you think about that or whether it's too complicated.

Comment on lines +279 to +281
if (process.env.BABEL_ENV === "test") {
throw new Error("Bucket was forcefully evicted/garbage-collected.");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess to avoid this you could check whether an ErrorHandling.notify stub was called with the correct error in the tests.

tools/test.sh Outdated Show resolved Hide resolved
tools/test.sh Outdated Show resolved Hide resolved
@philippotto
Copy link
Member Author

After fixing a new race condition, I found yet another one. I pushed a fix, but wouldn't want to merge it today/tomorrow. Since I'm on vacation next week, it's probably a good idea to have another look afterwards to reduce the risk of any other problems 🙈

@philippotto
Copy link
Member Author

Hm, the tests succeed locally for me, but the CI fails for the new Brushing/Tracing should send buckets to backend and restore dirty flag afterwards test 🤷 Maybe I'll have an idea about this after my vacation.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@philippotto philippotto enabled auto-merge (squash) September 27, 2021 13:32
@philippotto
Copy link
Member Author

Closing this since the changes are included in #5733.

philippotto added a commit that referenced this pull request Oct 12, 2021
…#5728 + #5733)

* remove dead and unnecessary code

* comment out uncalled push function

* add more volume-related tests and fix two bugs

* add console.error if async task queue fails

* fix tests, add comments and add crash-provoking test

* make dynamc overwriting of MAXIMUM_BUCKET_COUNT_PER_LAYER work in spec

* fix test.sh

* reference issue for bug/todo

* update changelog

* fix linting

* fix tests

* fix --timeout param for test.sh

* use snapshot tests in volume tracing tests and clean up test.sh

* Update tools/test.sh

Co-authored-by: Daniel <[email protected]>

* fix race condition in unsetting dirty flag

* WIP: implement first version of flood fill (resampling is still broken)

* fix z index

* refactor flood fill code and fix some bugs

* fix flow

* fix more xyz/uvw bugs

* simplify code which increments dirtyCount

* limit 3d flood fill to certain bounding box, inform user whether the limit was reached, show progress for filling and further fixes for 3d flood fill

* build a test which crashes undo because getData() was called on a gc-ed bucket

* fix crashing undo if bucket was collected in meantime (see #5729) and add progress indicator to undo/redo

* fix incorrectly mocked bucket data, properly await undo/redo and clean up

* use redux channels to avoid missing critical actions, such as undo or volume-related actions, when being busy; also make undo/redo buttons AsyncClickable

* clean up

* fix flow

* fix building docker image in CI

* add more comments to applyAndGetRevertingVolumeBatch

* rename action channels

* rename VoxelNeighborStack3D to VoxelNeighborQueue3D

* run all volume saga integration specs

* clean up tests

* allow to switch between 2D and 3D for fill tool

* fix failing mesh loading for hybrid tracings

* implement smart bbox growing for flood fill and create bbox for flood fill limit if it was reached

* fix volumetracing_saga.spec

* fix abort criterium in flood fill; add warning if bucket was given too small array buffer; fix off-by-one in covered floodfill bbox

* fix test for bucket garbage collection

* mark UI as busy when undo/redo/floodfill are computing

* fix tests by clearing stale push and pull queues properly when reinitializing the model

* update snapshot

* fix flow

* incorporate PR feedback (1/x)

* incorporate more feedback

* don't show progress handling if undo stack is empty and refactor corresponding code

* inform user if floodfill bbox was added and show that message for 10s

* update progress UI in-place, also write source_id into bbox name, don't add to undo stack if floodfill is no-op, fix flow and clean up

* only show undo/redo progress message for volume actions

* minor tweaks

* add a comment for USE_FLOODFILL_VOXEL_THRESHOLD

* intersect floodfill bbox with datasets bounding box and add snapshot test

* decrease snapshot size

* fix style of disabled undo/redo buttons

* improve comments

* make floodfill bbox message closable and deduplicate debug info code

* adapt antd typing according to webknossos/pull/5738

Co-authored-by: Daniel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants