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 LOD mesh support for frontend #6909

Merged
merged 24 commits into from
Mar 30, 2023
Merged

Add LOD mesh support for frontend #6909

merged 24 commits into from
Mar 30, 2023

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Mar 9, 2023

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • test local -> add the mesh file with the new format and old format into the l4_sample datasets' segmentation folder l4_sample/segmentation/meshes/.... The meshfiles are available in the following slack thread: https://scm.slack.com/archives/C5AKLAV0B/p1674140733622169?thread_ts=1674138004.502719&cid=C5AKLAV0B
  • View the l4_sample dataset
  • load a mesh with the new meshfile format (precomputed)
  • load a mesh with the old meshfile format (precomputed)
  • try reloading the meshes
  • hide the meshes
  • download the meshes
  • zoom in and out. all meshes should be always visible.
  • delete the meshes
  • and more mesh interactions that you can think of :)

Issues:


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

@MichaelBuessemeyer MichaelBuessemeyer changed the title [WIP] Add LOD mesh support for frontend Add LOD mesh support for frontend Mar 9, 2023
@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review March 9, 2023 18:21
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 stuff 👍 Only have some smaller remarks mostly. One cool thing would be progressive rendering per segment (or per segment chunk), but I'm not sure whether the current scene graph hierarchy is suitable for that. Let me know what you think!

Copy link
Contributor Author

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

I think I covered all open comments, right? (besides the testing of the constants that determine the current LOD).

But a review of the backend part is still missing, right?

@fm3
Copy link
Member

fm3 commented Mar 29, 2023

But a review of the backend part is still missing, right?

The backend changes look more complex than they are. I only renamed a few things (with the help of the IDE), added the one factor to fix the mag scaling (which we tested) and changed the signatures of some things from Int to Long (which we tested). Since backend reviewers are a scarce resource currently, I’d say it’s fine to just ship it.

@philippotto
Copy link
Member

philippotto commented Mar 29, 2023

Awesome! Thanks for implementing the custom raycaster. This should be a good boost for performance :)

Some comments:

  • for mesh files that don't have multiple lods, the meshes are disappearing when zooming out. in general, the lod code should take into account how many LODs exist (currently, it always assumes 3, but it could be 1 or 5 or 10 ...)
  • I'd rename the raycaster module and class to something like VisibilityAwareRaycaster
  • Regarding the generalization of the lod selection, I'll come back to you. However, what I'd do in either way is to adapt the logic so that there is an array of zoom thresholds which is iterated then. Something like this:
const LOD_THRESHOLDS = [0.7, 3];

...
let currentIndex = 0;
while (scale > LOD_THRESHOLDS[currentIndex] && currentIndex < availableLodCount) {
  currentIndex++;
  if (currentIndex === LOD_THRESHOLDS.length) {
    // generate next thresholds as necessary
    LOD_THRESHOLDS.push(2 * LOD_THRESHOLDS[currentIndex - 1])
  }
}
return currentIndex;

(not tested, but I hope the gist is comprehensible)

@MichaelBuessemeyer
Copy link
Contributor Author

for mesh files that don't have multiple lods, the meshes are disappearing when zooming out. in general, the lod code should take into account how many LODs exist (currently, it always assumes 3, but it could be 1 or 5 or 10 ...)

But mesh files that do not have version 3 still work and do not disappear, right? Were you able to trigger some kind of bug like this? I assume you mean mesh version 3 files that only have one or two LODs, right?

Regarding the generalization of the lod selection, I'll come back to you. However, what I'd do in either way is to adapt the logic so that there is an array of zoom thresholds which is iterated then. Something like this:

Thanks for coming up with this heuristic suggestion 🎉

I changed your suggested code a little. Now the custom LOD class maintains a list of thresholds and automatically adds new thresholds if a mesh with a LOD is added that is not supported by the current threshold list.

I also tested the mechanism of adding thresholds depending on the LOD count of the meshes. I simply changed this.lodThresholds = [0.7, 3]; to this.lodThresholds = [0.7]; and it worked without any issues. The only side effect was that the switching between different LODs was done on smaller levels of scale and thus visible to the eye. But as this was just a test and because we actually use 3 for the 2nd threshold, I think is fine.

One bug I can imagine is that if a mesh is loaded that supports 3 LODs and then a mesh with 5 LODs is loaded, the first mesh will be invisible on very small scales / far zoomed out. But this is a pretty hypothetical scenario right now in my opinion. So I wouldn't tackle this right now.
But this should be easy to solve by remembering the max LOD for each mesh and ensuring that a meshes max LOD is rendered at least :) (Although this could dip the performance quite a bit with a lot of meshes)

@philippotto
Copy link
Member

for mesh files that don't have multiple lods, the meshes are disappearing when zooming out. in general, the lod code should take into account how many LODs exist (currently, it always assumes 3, but it could be 1 or 5 or 10 ...)

But mesh files that do not have version 3 still work and do not disappear, right? Were you able to trigger some kind of bug like this? I assume you mean mesh version 3 files that only have one or two LODs, right?

I could reproduce the error here with meshfile_v3 (which doesn't have lods apparently). But now it works due to your code change :)

Regarding the generalization of the lod selection, I'll come back to you. However, what I'd do in either way is to adapt the logic so that there is an array of zoom thresholds which is iterated then. Something like this:

Thanks for coming up with this heuristic suggestion 🎉

I changed your suggested code a little. Now the custom LOD class maintains a list of thresholds and automatically adds new thresholds if a mesh with a LOD is added that is not supported by the current threshold list.

Awesome!

I also tested the mechanism of adding thresholds depending on the LOD count of the meshes. I simply changed this.lodThresholds = [0.7, 3]; to this.lodThresholds = [0.7]; and it worked without any issues. The only side effect was that the switching between different LODs was done on smaller levels of scale and thus visible to the eye. But as this was just a test and because we actually use 3 for the 2nd threshold, I think is fine.

Yes, sounds okay to me 👍

One bug I can imagine is that if a mesh is loaded that supports 3 LODs and then a mesh with 5 LODs is loaded, the first mesh will be invisible on very small scales / far zoomed out. But this is a pretty hypothetical scenario right now in my opinion. So I wouldn't tackle this right now. But this should be easy to solve by remembering the max LOD for each mesh and ensuring that a meshes max LOD is rendered at least :) (Although this could dip the performance quite a bit with a lot of meshes)

I think it's a fair assumption for now that all meshes share the same LOD count. Maybe we need to rework the LOD hierarchy in the future, anyway (e.g., to allow that one mesh is rendered in LOD 0 and another one in LOD 3), then this might be solved in the same breath, but let's take it step by step.

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.

Awesome! Let's ship it, I'd say :)

@MichaelBuessemeyer MichaelBuessemeyer merged commit c857eed into master Mar 30, 2023
hotzenklotz added a commit that referenced this pull request Apr 3, 2023
…come-toast

* 'master' of github.com:scalableminds/webknossos:
  Log all details on deleting annotation layer (#6950)
  fix typo
  Rename demo instance to wkorg instance (#6941)
  Add LOD mesh support for frontend (#6909)
  Fix layout of view mode switcher and move it (#6949)
  VaultPath no longer extends nio.Path (#6942)
  Release 23.04.0 (#6945)
  Use new zip.js version to allow zip64 uploads (#6939)
  Implement viewing sharded neuroglancer precomputed datasets (#6920)
  Reject dataset uploads if organization storage quota is exceeded (#6893)
  Refactor deprecated antd Dropdown menus (#6898)
hotzenklotz added a commit that referenced this pull request Apr 4, 2023
…wings

* 'master' of github.com:scalableminds/webknossos:
  updates docs for docker installation (#6963)
  Fix misc stuff when viewing tasks/annotations of another user (#6957)
  Remove segment from list and add undo/redo for segments (#6944)
  Log all details on deleting annotation layer (#6950)
  fix typo
  Rename demo instance to wkorg instance (#6941)
  Add LOD mesh support for frontend (#6909)
  Fix layout of view mode switcher and move it (#6949)
  VaultPath no longer extends nio.Path (#6942)
  Release 23.04.0 (#6945)
  Use new zip.js version to allow zip64 uploads (#6939)
  Implement viewing sharded neuroglancer precomputed datasets (#6920)
  Reject dataset uploads if organization storage quota is exceeded (#6893)
  Refactor deprecated antd Dropdown menus (#6898)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants