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

Improve performance for large (oversegmentation) meshes #7077

Merged
merged 5 commits into from
May 16, 2023

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented May 15, 2023

Avoid triggering the raycasting when rotating or moving the 3d viewport to avoid lags.
Merge buffer geometries per chunk when loading oversegmentation meshes to avoid lag due to too many threejs scene objects.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Test that loading ad-hoc and precomputed meshes from non-oversegmentation mesh files still works
  • See the gifs that were shared internally for comparison
  • I could also provide the dataset I used to test with, but it would be ~4 GB compressed

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

@daniel-wer daniel-wer self-assigned this May 15, 2023
@daniel-wer daniel-wer requested a review from philippotto May 15, 2023 14:54
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.

code looks perfect 👍 i didn't test yet, as I have one suggestion which might make sense to do before testing:

Since you implemented the merging of meshes in one chunk, the issue #6519 just got even easier. I think, it would only be a matter of adding a line à la geometry = mergeVertices(geometry);.

Do you feel like giving this a try to see whether:

  • this reduces visible mesh borders
  • the performance hit is negligible

If you don't feel like tacking this on this PR, it's also fine. Just let me know; then, I'll test the PR :)

@daniel-wer
Copy link
Member Author

daniel-wer commented May 15, 2023

Since you implemented the merging of meshes in one chunk, the issue #6519 just got even easier. I think, it would only be a matter of adding a line à la geometry = mergeVertices(geometry);.

Do you feel like giving this a try to see whether:

  • this reduces visible mesh borders
  • the performance hit is negligible

I tested that, see this log for some time measurements and how many vertices were removed for some sample chunks for lods 0 and 4:

console.js:13 Before 9045 lod 0
isosurface_saga.ts:863 simplification: 50.305908203125 ms
console.js:13 After 8859 lod 0
console.js:13 Before 7085 lod 0
isosurface_saga.ts:863 simplification: 29.9140625 ms
console.js:13 After 6932 lod 0
console.js:13 Before 12333 lod 0
isosurface_saga.ts:863 simplification: 73.60693359375 ms
console.js:13 After 11944 lod 0
console.js:13 Before 12137 lod 0
isosurface_saga.ts:863 simplification: 115.741943359375 ms
console.js:13 After 11763 lod 0
console.js:13 Before 12848 lod 4
isosurface_saga.ts:863 simplification: 84.89111328125 ms
console.js:13 After 10499 lod 4
console.js:13 Before 13198 lod 4
isosurface_saga.ts:863 simplification: 62.898193359375 ms
console.js:13 After 12019 lod 4
console.js:13 Before 13172 lod 4
isosurface_saga.ts:863 simplification: 62.249755859375 ms
console.js:13 After 11403 lod 4
console.js:13 Before 10787 lod 4
isosurface_saga.ts:863 simplification: 88.73193359375 ms
console.js:13 After 9325 lod 4
console.js:13 Before 11184 lod 4
isosurface_saga.ts:863 simplification: 55.5029296875 ms
console.js:13 After 9816 lod 4

For the large mesh I'm testing with, this would be called thousands of times so the performance impact would be noticeable I'd say. For the in-chunk borders, I don't see any difference :/

Before:
chunk_borders_before
After:
chunk_borders_after

This was the diff for the experiment

diff --git a/frontend/javascripts/oxalis/model/sagas/isosurface_saga.ts b/frontend/javascripts/oxalis/model/sagas/isosurface_saga.ts
index 570f6be657..d5c083bf8a 100644
--- a/frontend/javascripts/oxalis/model/sagas/isosurface_saga.ts
+++ b/frontend/javascripts/oxalis/model/sagas/isosurface_saga.ts
@@ -853,15 +853,29 @@ function _getLoadChunksTasks(
                   // If mergeBufferGeometries does not succeed, the method logs the error to the console and returns null
                   if (geometry == null) continue;

-                  // Compute vertex normals to achieve smooth shading
-                  geometry.computeVertexNormals();
+                  // Delete existing vertex normals (since these are not interpolated
+                  // across faces).
+                  geometry.deleteAttribute("normal");
+                  // Ensure that vertices of adjacent faces are shared.
+                  console.log("Before", geometry.getAttribute("position").count, "lod", lod);
+                  console.time("simplification");
+                  const simplifiedGeometry = mergeVertices(geometry);
+                  console.timeEnd("simplification");
+                  console.log(
+                    "After",
+                    simplifiedGeometry.getAttribute("position").count,
+                    "lod",
+                    lod,
+                  );
+                  // Recompute normals to achieve smooth shading
+                  simplifiedGeometry.computeVertexNormals();

                   yield* call(
                     {
                       context: segmentMeshController,
                       fn: segmentMeshController.addIsosurfaceFromGeometry,
                     },
-                    geometry,
+                    simplifiedGeometry,
                     id,
                     position,
                     // Apply the scale from the segment info, which includes dataset scale and mag

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 🎉 🎉 🎉

I also tested the performance and for me the FPS went from 15 to 40 when using the rotate-buttons for a scene with three large-ish meshes. I also noticed that the loading speed of the meshes was faster, but maybe this was imagination (?)

Also thank you for giving the mergeVertices idea a go 👍 I commented in the original issue that we probably need something that merges across chunks. And even that, it's probably a trade-off between performance and looks.

@daniel-wer daniel-wer merged commit 4ce46bd into master May 16, 2023
@daniel-wer daniel-wer deleted the improve-laggy-meshes branch May 16, 2023 11:36
hotzenklotz added a commit that referenced this pull request May 17, 2023
…ty-list-drawings

* 'master' of github.com:scalableminds/webknossos: (23 commits)
  fix scrolling in organization switcher in terms-of-services modal (#7083)
  Auto-Select via SAM (#7051)
  Build STL in chunks when exporting them (#7074)
  Improve performance for large (oversegmentation) meshes (#7077)
  Fix display of used storage in power plan (#7057)
  Fix user limits for invites (#7078)
  adds fileSize to voxelytics workflow list (#7071)
  Improve error logging for bucket requests (#7053)
  Fix zarr streaming datasource-properties.json generation for non-wkw/zarr datasets (#7065)
  Min length for layer names is one (#7064)
  Include voxelytics workflow name in tab title (#7070)
  Fix local to global layer index look up (#7066)
  Merge editable mappings (#7026)
  store correct artifacts for wkorg nightly (#7060)
  Team edit modal (#7043)
  Fix voxel offset in chunk name for unsharded neuroglancer precomputed datasets (#7062)
  Fix offset when loading non-aligned buckets for zarr/n5/precomputed (#7058)
  fixes wallTimes query for workflow reports (#7059)
  Handle Remote Dataset Edge Cases: compressed content-encoding, float voxel size (#7041)
  Release 23.05.2 (#7056)
  ...
hotzenklotz added a commit that referenced this pull request May 17, 2023
…ove_wkconnect

* 'master' of github.com:scalableminds/webknossos: (33 commits)
  Fix bug in fallback rendering when layer has missing mags (#7082)
  Fix duplicate on volumes with no fallback layer (#7085)
  fix scrolling in organization switcher in terms-of-services modal (#7083)
  Auto-Select via SAM (#7051)
  Build STL in chunks when exporting them (#7074)
  Improve performance for large (oversegmentation) meshes (#7077)
  Fix display of used storage in power plan (#7057)
  Fix user limits for invites (#7078)
  adds fileSize to voxelytics workflow list (#7071)
  Improve error logging for bucket requests (#7053)
  Fix zarr streaming datasource-properties.json generation for non-wkw/zarr datasets (#7065)
  Min length for layer names is one (#7064)
  Include voxelytics workflow name in tab title (#7070)
  Fix local to global layer index look up (#7066)
  Merge editable mappings (#7026)
  store correct artifacts for wkorg nightly (#7060)
  Team edit modal (#7043)
  Fix voxel offset in chunk name for unsharded neuroglancer precomputed datasets (#7062)
  Fix offset when loading non-aligned buckets for zarr/n5/precomputed (#7058)
  fixes wallTimes query for workflow reports (#7059)
  ...
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