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

Use new mesh API for animation job #7692

Merged
merged 21 commits into from
Apr 24, 2024
Merged

Use new mesh API for animation job #7692

merged 21 commits into from
Apr 24, 2024

Conversation

hotzenklotz
Copy link
Member

@hotzenklotz hotzenklotz commented Mar 14, 2024

This PR adds support for passing the required mesh information to use mesh API (PR #7587)for animation jobs.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Set up worker with PR https://github.com/scalableminds/voxelytics/pull/3472
  • Enable worker in WK
  • Test view mode with a dataset containing a segmentation.
    • Both ad-hoc and pre-computed meshes should work
  • Test annotation:
    • Load some pre-computed meshes for an existing segmentation layer
    • Load some ad-hoc meshes for an existing segmentation layer
    • Create a second volume layer, brush some and load the resulting meshes
  • If you are really diligent:
    • Test switching the ad-hoc meshing mag quality
    • Load meshes from different pre-computed meshfiles

TODOs:

  • AdHoc Meshing
  • Testing with mappings etc

Issues:


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

@hotzenklotz hotzenklotz self-assigned this Mar 14, 2024
@hotzenklotz hotzenklotz marked this pull request as draft March 14, 2024 14:34
@hotzenklotz hotzenklotz marked this pull request as ready for review March 18, 2024 13:34
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.

didn't test yet, but looks good already 👍 added some minor comments.

docs/animations.md Outdated Show resolved Hide resolved
frontend/javascripts/types/api_flow_types.ts Outdated Show resolved Hide resolved
}
}
// Submit currently visible pre-computed & ad-hoc meshes
const axis = "";
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 axis = "";
// Currently, additional coordinates are not supported for this rendering job
const axis = getAdditionalCoordinatesAsString([]);

would be a bit cleaner.

"layer_name" -> animationJobOptions.layerName,
"segmentation_layer_name" -> animationJobOptions.segmentationLayerName,
"is_view_mode" -> animationJobOptions.isViewMode,
"color_layer_name" -> animationJobOptions.colorLayerName,
Copy link
Member

Choose a reason for hiding this comment

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

note that if the command args change, the job list view code in the frontend also needs to be adapted (currently the existing layer_name is used to render the job description. compare also getJobs in admin_rest_api

Copy link
Member

Choose a reason for hiding this comment

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

we also need to ensure backwards compatibility for this job list view rendering (or migrate old jobs in postgres)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I wasn't aware of this connection... will undo the renaming then.

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

I tested again and most cases work for me now 🎉

I noticed two more small problems

This message already crops up on pageload when viewing a dataset without color layers. It would be nice if that would happen only once you actually clicked on create animation.

image

And: sometimes seedAdditionalCoordinates is not included in the mesh info json (in my test this happened in a proofreading annotation). Since the python code expects this field to be present (although null is a valid option), it fails at that point. Not sure if it should be adapted here or there.

@hotzenklotz
Copy link
Member Author

I noticed two more small problems

This message already crops up on pageload when viewing a dataset without color layers. It would be nice if that would happen only once you actually clicked on create animation.

I fixed this.

And: sometimes seedAdditionalCoordinates is not included in the mesh info json (in my test this happened in a proofreading annotation). Since the python code expects this field to be present (although null is a valid option), it fails at that point. Not sure if it should be adapted here or there.

I have a hard time to reproduce this. I played around with proofreading annotations but the request always includes a an empty array for the seedAdditionalCoordinates which should be fine?

@philippotto
Copy link
Member

Since the python code expects this field to be present (although null is a valid option) [...]

I played around with proofreading annotations but the request always includes a an empty array for the seedAdditionalCoordinates which should be fine?

I think, it would be easiest to adapt the python code so that a missing value is also okay. Alternatively, we would need to turn seedAdditionalCoordinates? into seedAdditionalCoordinates in BaseMeshInformation, but that probably requires touching several code spots.

Without doing one of these options, rare hiccups are bound to happen at some point, since the property is typed as "may be missing".

@hotzenklotz hotzenklotz merged commit 16e64da into master Apr 24, 2024
1 check passed
@hotzenklotz hotzenklotz deleted the animation_meshes_api branch April 24, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants