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

New Mesh File Support #6491

Merged
merged 73 commits into from
Oct 5, 2022
Merged

New Mesh File Support #6491

merged 73 commits into from
Oct 5, 2022

Conversation

leowe
Copy link
Contributor

@leowe leowe commented Sep 22, 2022

URL of deployed dev instance (used for testing):

Steps to test:

  • Use a mesh file with the new format and use "Show precomputed mesh" for a segment in webknossos

Issues:


  • Remove Draco dummy route & functions

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

@philippotto
Copy link
Member

@daniel-wer I think you can already review the PR. For the lighting part, I'm still waiting for other feedback, but that code isn't really critical anyway. For the version comparison with 2, I'm a bit hesitant, since I know that there are already mesh files in circulation with v=2. So, maybe it's alright to leave the comparison at >= 2. What do you think?

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.

Nice work, I will report back after testing 👍

frontend/javascripts/oxalis/model/sagas/isosurface_saga.ts Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/model/sagas/isosurface_saga.ts Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/model/sagas/isosurface_saga.ts Outdated Show resolved Hide resolved
meshFileName,
id,
);
// todo: should actually check against 3, but some new mesh files
Copy link
Member

Choose a reason for hiding this comment

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

For the version comparison with 2, I'm a bit hesitant, since I know that there are already mesh files in circulation with v=2. So, maybe it's alright to leave the comparison at >= 2. What do you think?

Are we talking about many (or important) mesh files with version 2? Otherwise, I would lean towards bumping the comparison to 3 and assert that the mesh file version is neither 1 nor 2. This would make the code easier to understand and forfeit the need to explain that everything is named v3 but some mesh files have v2. Also we would need to make sure that there exists a test dataset that has a mesh file with the correct version 3.

frontend/javascripts/types/api_flow_types.ts Outdated Show resolved Hide resolved
public/wasm/draco_wasm_wrapper.js Show resolved Hide resolved
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.

The mesh loading with new and old mesh file formats worked very well 👍

I only noticed one issue during testing. When opening a sharing link that included an active mesh file, for example, this one: https://newmeshfilesupport.webknossos.xyz/annotations/633d73740100009b005f634a#%7B%22position%22:%5B3517,3800,1025%5D,%22mode%22:%22orthogonal%22,%22zoomStep%22:1,%22stateByLayer%22:%7B%22071103ab-b959-4d83-9538-d7512ad225c9%22:%7B%22meshInfo%22:%7B%22meshFileName%22:%22meshfile_v3%22,%22meshes%22:%5B%7B%22segmentId%22:58624,%22seedPosition%22:%5B3436,3687,1025%5D,%22isPrecomputed%22:true,%22meshFileName%22:%22meshfile_v3%22%7D%5D%7D%7D%7D%7D

the other mesh file is activated, not the "meshfile_v3". (The mesh that is encoded in the URL as well is loaded using the correct mesh file!)

@philippotto
Copy link
Member

the other mesh file is activated, not the "meshfile_v3". (The mesh that is encoded in the URL as well is loaded using the correct mesh file!)

good catch! I fixed it now :) if you give your final "go", I'd merge this 🚢

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.

LGTM, please see the code suggestions to fix a remaining typo. 👍

// light2.position.set(-1, -1, -1).normalize();
// this.isosurfacesRootGroup.add(light2);
const AMBIENT_INTENSITY = 30;
const DIRECTIONAL_INTENSITRY = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const DIRECTIONAL_INTENSITRY = 5;
const DIRECTIONAL_INTENSITY = 5;


const ambientLight = new THREE.AmbientLight(2105376, AMBIENT_INTENSITY);

const directionalLight = new THREE.DirectionalLight(16777215, DIRECTIONAL_INTENSITRY);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const directionalLight = new THREE.DirectionalLight(16777215, DIRECTIONAL_INTENSITRY);
const directionalLight = new THREE.DirectionalLight(16777215, DIRECTIONAL_INTENSITY);

directionalLight.position.z = 1;
directionalLight.position.normalize();

const directionalLight2 = new THREE.DirectionalLight(16777215, DIRECTIONAL_INTENSITRY);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const directionalLight2 = new THREE.DirectionalLight(16777215, DIRECTIONAL_INTENSITRY);
const directionalLight2 = new THREE.DirectionalLight(16777215, DIRECTIONAL_INTENSITY);

frontend/javascripts/oxalis/model/sagas/isosurface_saga.ts Outdated Show resolved Hide resolved
@philippotto philippotto enabled auto-merge (squash) October 5, 2022 14:24
@philippotto philippotto enabled auto-merge (squash) October 5, 2022 14:24
…write the existing key), as this is important when an existing precomputed mesh is changed to an ad-hoc one (the same was already done when adding precomputed meshes)
@philippotto philippotto merged commit ea424ac into master Oct 5, 2022
@philippotto philippotto deleted the new-mesh-file-support branch October 5, 2022 15:05
@fm3 fm3 restored the new-mesh-file-support branch October 12, 2022 08:04
@fm3 fm3 deleted the new-mesh-file-support branch October 12, 2022 08:05
hotzenklotz added a commit that referenced this pull request Oct 13, 2022
…jects-created

* 'master' of github.com:scalableminds/webknossos: (337 commits)
  Fix docs for the annotation download file format (#6546)
  Added total runtime information to VX reports (#6543)
  fix VX report for completed + skipped tasks (#6540)
  Avoid allocating spire uint objects during apply agglomerate (#6532)
  Explore remote N5 datasets (#6520)
  Fix MeshChunk byteOffset (Long, not Int) (#6536)
  update browserslist (#6505)
  Support new Mesh File (v3) (#6491)
  makes workflow_yamlContent optional (#6518)
  Always return 404 for Failures in Zarr Streaming (#6515)
  Poll wk version to notify during upgrade (#6451)
  add script which extracts newest changelog and creates GH release for it (#6504)
  release 22.10.0 (#6500)
  voxel³ -> voxel (#6501)
  Allow task type summary to identify task type when creating tasks in bulk (#6486)
  Fix sql evolution 090 (defer not null constraint) (#6498)
  SQL schema cleanup (#6492)
  Fix validation of layer selection when trying to start globalization of floodfills (#6497)
  Add "shift + w" shortcut to cycle backwards through tools (#6493)
  Fix filtering for public datasets in dataset table (#6496)
  ...
@philippotto philippotto restored the new-mesh-file-support branch October 18, 2022 07:58
@fm3 fm3 deleted the new-mesh-file-support branch January 27, 2023 10:15
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.

Support new Mesh File Format
4 participants