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

Add loaded meshes to sharing link #5993

Merged
merged 39 commits into from
Feb 21, 2022
Merged

Add loaded meshes to sharing link #5993

merged 39 commits into from
Feb 21, 2022

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Jan 26, 2022

  • Includes some cleanup (review first commit separately to have an isolated look)
    • I was annoyed that because flow returns mixed for Object.values, we often had to add $FlowIssue comments, or worse, adapted the code to work around that by using Object.keys, converting the key to int and then mapping to get the values. I introduced Utils.values which returns the correct type and should be the only spot where the $FlowIssue comment is needed. (_.values also seems to be correctly typed, but has unknown overhead)
    • A couple of times I specified the type if the values were mapped, afterwards, which is a separate issue, since mapping Array<number> will lead to flow assuming a type of number | any for the mapped value argument 🤷 I did not systematically look for these occurrences, but I assume there are quite a few in our code, leading to less type security.
    • Flow Coverage improved:
      Before │project │ percent │  total │ covered │ uncovered │
             │ oxalis │    88 % │ 145209 │  127907 │     17302 │
      After  │project │ percent │  total │ covered │ uncovered │
             │ oxalis │    88 % │ 145166 │  127953 │     17213 │
      
  • I cleaned up the isosurface related code quite a bit. Most prominently, I removed the functionality where flycam position changes triggered a continuation of isosurface loading under some circumstances. The respective code was triggered quite a lot (and usually for nothing) and we decided that it is obscure and not really useful. I also removed some unused code and parameters.
  • I converted the precomputed mesh loading code to a saga, so it can be easily triggered by an action call and also easily be canceled if the user deletes the mesh while it's loading.
  • The loaded meshes are now included in the sharing link and are automatically loaded if such a link is opened. I included only the currently visible meshes in the sharing link.
  • Sort the precomputed mesh chunks by their distance to the seedPosition before loading them, so that the mesh loads from the seedPosition out.

URL of deployed dev instance (used for testing):

Steps to test:

  • Test mesh related functionality (both in a tracing as well as in view mode!)
    • Load ad-hoc/precomputed meshes
    • Download meshes
    • Reload ad-hoc meshes
    • Clicking on mesh entries should jump to the seed position
  • Test that meshes can be shared via link
    • Load a couple of meshes (ad-hoc and precomputed)
    • Copy the sharing link from the share modal or by clicking the button right of the position view
    • Open a new tab and enter the url -> The meshes that were visible before should be automatically loaded
  • Test that mesh loading is aborted if the mesh is deleted while loading
    • Open the network tab and throttle the network speed
    • Load meshes and delete them while loading
  • Test refreshIsosurfaces api method
    • Open a volume tracing without a fallback layer
    • Brush 2 different segments (4 slices, otherwise you might not get a mesh!)
    • Compute ad-hoc meshes for both of them
    • Modifiy one of the segments
    • Call window.webknossos.apiReady(3).then(api => api.data.refreshIsosurfaces());
    • -> Only the modified isosurface should be reloaded

Issues:


(Please delete unneeded items, merge only when none are left open)

…ean up usages of Object.{values|keys} and improve flow typing
@daniel-wer daniel-wer self-assigned this Jan 26, 2022
…ycam position change, DRY isosurface refresh code, refactor and rename changeActiveIsosurfaceCell to loadAdHocMesh, remove unused isosurface actions
@daniel-wer daniel-wer changed the title [WIP] Add loaded meshes to sharing link Add loaded meshes to sharing link Jan 27, 2022
@daniel-wer
Copy link
Member Author

@philippotto I have mostly tested the happy paths for this PR, but will do some more monkey testing next week. Still, this should be ready for a first review :)

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Great work & refactoring 👍 It's a joy to read through these changes 🕺

I only have some concerns with processTaskWithPool. Maybe they are unfounded (I didn't test my assumption), but my current understanding is that this needs some changes.. Happy to call about this, if you want 🤙

frontend/javascripts/oxalis/api/api_latest.js Show resolved Hide resolved
frontend/javascripts/oxalis/controller/url_manager.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/controller/url_manager.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/action-bar/save_button.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/model_initialization.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/model_initialization.js Outdated Show resolved Hide resolved
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Great adaptation of the pool "saga" 🥇 I couldn't get the sharing link to work, yet, though. I tried it with adhoc meshes and the URL doesn't seem to contain anything mesh related 🤔

frontend/javascripts/libs/task_pool.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/model/sagas/isosurface_saga.js Outdated Show resolved Hide resolved
…e meshes are loaded, adapt types accordingly, fix that reloading a mesh reloaded it with the current mapping instead of the original one
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

I wasn't sure, whether the new round is ready for review again, but especially the last commit seemed like a bigger chunk ready to review :) Great work refactoring this! I only left two suggestions. Let me know when/whether I should also test again :)

frontend/javascripts/oxalis/model/sagas/isosurface_saga.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/model_initialization.js Outdated Show resolved Hide resolved
@daniel-wer
Copy link
Member Author

I wasn't sure, whether the new round is ready for review again, but especially the last commit seemed like a bigger chunk ready to review :)

Thanks a lot!

I addressed your feedback, would be great if you could do another test round.
I'll also test some more and will add a screenshot test as well.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Sorry, this PR slipped through on my side! I reviewed the latest commits and also did another testing round. Looks very solid 👍 Looking forward to see this live 🎉

@daniel-wer
Copy link
Member Author

Nightly test ran through well: https://app.circleci.com/pipelines/github/scalableminds/webknossos/5717/workflows/22bbe067-6ff4-4a38-a907-c37a4132ee19/jobs/17004

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Wonderful! Thank you so much for taking care of the screenshot tests 🙏 I reviewed the latest commits and everything looks good!

threshold: 0.0,
});
changedPixels = pixelmatch(
// The buffers need to be converted to png before comparing them
Copy link
Member

Choose a reason for hiding this comment

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

Has this become necessary due to the update to pixelmatch? Or has this always been an issue? Good catch, either way 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I read and experienced, pixelmatch started to assert this in the new version but the problem existed before. Usually the difference was fairly small (which is why it mostly worked before), but I also encountered garbled diffs during my testing with other --use-gl flags.

@daniel-wer daniel-wer merged commit 2854aab into master Feb 21, 2022
@daniel-wer daniel-wer deleted the meshes-in-link branch February 21, 2022 12:36
hotzenklotz added a commit that referenced this pull request Feb 21, 2022
…s_lili

* 'master' of github.com:scalableminds/webknossos:
  Add loaded meshes to sharing link (#5993)
hotzenklotz added a commit that referenced this pull request Feb 22, 2022
* 'master' of github.com:scalableminds/webknossos:
  Release 22.03 (#6069)
  Skip deactivated users in dataset access list (#6070)
  Compile tests to different dir to avoid unnecessary recompilations (#6065)
  Add loaded meshes to sharing link (#5993)
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