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 Render Animation Worker Job #7348

Merged
merged 39 commits into from
Oct 23, 2023
Merged

Add Render Animation Worker Job #7348

merged 39 commits into from
Oct 23, 2023

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Sep 19, 2023

This PR adds the frontend and backend feature for starting a animation job of the current annotation / dataset.

image

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Start a worker with animation job (Export 3D render via worker job #7338)
  • Test in View mode and for an annotation
  • For "Menu" in navbar select "Create Animation"
  • Play around with available options to create various animations
  • Restrict your orga to "Basic" plan and check plan enforcement

Issues:


@fm3 fm3 self-assigned this Sep 19, 2023
@fm3 fm3 marked this pull request as draft September 19, 2023 12:10
@hotzenklotz hotzenklotz mentioned this pull request Sep 28, 2023
7 tasks
@hotzenklotz hotzenklotz changed the title WIP: Add Render Animation Worker Job Add Render Animation Worker Job Oct 17, 2023
@@ -684,6 +690,18 @@ class TracingActionsView extends React.PureComponent<Props, State> {
}

menuItems.push(screenshotMenuItem);

if (activeUser?.isSuperUser) {
Copy link
Member

Choose a reason for hiding this comment

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

This is by design. Let's do a soft launch and try some larger datasets on the server first before releasing this publicly.

@hotzenklotz
Copy link
Member

@philippotto I think this is ready for review. Feel free to only review the WK side. In case you setup up the worker too, perhaps you can do a rough review for that as well. Thanks.

I will create a page for the WK documentation later this week in a separate PR. Let's try to get this merged and soft launched for super users so that we can iron out any issue with the worker deployment and test performance in the real world.

@hotzenklotz hotzenklotz requested review from philippotto and hotzenklotz and removed request for hotzenklotz October 17, 2023 08:12
@fm3
Copy link
Member Author

fm3 commented Oct 17, 2023

I’d say the backend changes are straightforward enough that they don’t require a specialized review. @philippotto maybe you could quickly read through them as well while checking the front-end

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 stuff 👍 only left a couple of nitpicks.

also, one thing UX wise: I'd put the word "video" somewhere to clarify that the output medium is a video file. otherwise, it might be unclear to the user what they will get (a gif? a blender file? an upload to youtube?).

Comment on lines 166 to 167
let meshFileName = "meshfile.hdf5";
let segmentationLayerName = "segmentatation";
Copy link
Member

Choose a reason for hiding this comment

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

what's up with these? why not initialize these with null and then show an error in case no mesh files were found later?
also, there's a typo in segmentation.

Copy link
Member

Choose a reason for hiding this comment

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

Mhm, the backend expects a string - hence the defaults. If any mesh is rendered is mostly controlled by the submitted segment IDs.

Showing an error is a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this is optional then I’d say the backend should also understand that. I can probably add that today.

Copy link
Member Author

@fm3 fm3 Oct 19, 2023

Choose a reason for hiding this comment

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

I rechecked the backend and worker code, and there the type is segmentationLayerName: Option[String] and meshFileName: Option[String], so it should be possible to make this optional in the frontend already.

Copy link
Member

Choose a reason for hiding this comment

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

I switched the frontend values to undefined by default.

@philippotto
Copy link
Member

I’d say the backend changes are straightforward enough that they don’t require a specialized review. @philippotto maybe you could quickly read through them as well while checking the front-end

I did that yesterday, too, and everything looked good to me :)

hotzenklotz and others added 5 commits October 18, 2023 13:42
…odal.tsx

Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
…odal.tsx

Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
@hotzenklotz
Copy link
Member

@fm3 Did you built email notifications for this job as well?

@fm3
Copy link
Member Author

fm3 commented Oct 19, 2023

I did not adapt the email code, no. My understanding is that a generic job completed email will be sent? Feel free to add a specialized email though :) Or I could do it as well if you tell me what it should say

@hotzenklotz
Copy link
Member

I did not adapt the email code, no. My understanding is that a generic job completed email will be sent? Feel free to add a specialized email though :) Or I could do it as well if you tell me what it should say

I haven't tried the email notifications on my local dev setup (not configured). Let's start a soft launch and figure this out as well.

@fm3
Copy link
Member Author

fm3 commented Oct 19, 2023

I haven't tried the email notifications on my local dev setup (not configured). Let's start a soft launch and figure this out as well.

Sounds fair :) Then I guess the only remaining blockers are the frontend code review comments by philipp

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.

Good stuff 👍

@hotzenklotz hotzenklotz merged commit 6ba55b6 into master Oct 23, 2023
2 checks passed
@hotzenklotz hotzenklotz deleted the render-animation branch October 23, 2023 08:05
@hotzenklotz
Copy link
Member

Follow Up Issue: #3350

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.

Export 3D render via worker job
3 participants