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 direct render+export and notifications #3220

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Conversation

ginazhouhuiwu
Copy link
Contributor

@ginazhouhuiwu ginazhouhuiwu commented Jun 13, 2024

Directly render and export things from the viewer, with notifications!

Uses a simple notification addition from nerfstudio-project/viser#225.

Screen.Recording.2024-06-12.at.10.17.01.PM.online-video-cutter.com.1.mp4

TODO:

  • thoroughly test export functionality
  • add error notifications if encountered during render/export

@ginazhouhuiwu ginazhouhuiwu changed the title Add direct render+export buttons and notifications Add direct render+export and notifications Jun 13, 2024
@maturk
Copy link
Collaborator

maturk commented Jun 13, 2024

Nice PR. @ginazhouhuiwu, is it possible to also add the option to "capture frame" or something similar, where only a single image is saved instead of a video.

@ginazhouhuiwu
Copy link
Contributor Author

that sounds like a great feature I will try implementing! Thanks for taking a look : )

@maturk
Copy link
Collaborator

maturk commented Jun 13, 2024

that sounds like a great feature I will try implementing! Thanks for taking a look : )

Yeah, it has been a feature that I have seen some users bring up a few times in the past, just to quickly take some snapshots from a certain angle (perhaps using the same camera intrinsics as the original training dataset, but I think that is configurable in the gui anyways). Just some additional thoughts ... :)

@kerrj
Copy link
Collaborator

kerrj commented Jun 13, 2024

Super cool! Would also be great to build out some functionality for previewing the render as it goes and perhaps canceling it. Would be cool to also integrate with viser's download functionality to download the final render directly to someone's machine :)

@ginazhouhuiwu
Copy link
Contributor Author

ginazhouhuiwu commented Jun 22, 2024

Local download! Supported for both render and exports now : )

Screen.Recording.2024-06-21.at.5.50.44.PM.mov

@ginazhouhuiwu
Copy link
Contributor Author

ginazhouhuiwu commented Aug 27, 2024

Some finishing touches with suggestions from @kerrj, namely disabling render button when render is already in progress or if no keyframes are added, only enabling download button once file is ready, and allowing render to be canceled. The generate command button is added back as an additional option.

This PR is ready once nerfstudio-project/viser#225 is and I need to clean pyright. EDIT: is ready now : )

Screen.Recording.2024-08-26.at.8.25.46.PM.1.1.mov

@ginazhouhuiwu ginazhouhuiwu marked this pull request as ready for review August 27, 2024 03:42
@ginazhouhuiwu ginazhouhuiwu requested a review from kerrj August 29, 2024 23:17
@@ -432,6 +455,11 @@ class BaseRender:
"""If true, checks line-of-sight occlusions when computing camera distance and rejects cameras not visible to each other"""
camera_idx: Optional[int] = None
"""Index of the training camera to render."""
kill_flag: List[bool] = field(default_factory=lambda: [False])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not expose this in the config? it feels like it should be an invisible class self._kill_flag variable, then the kill() function sets that to true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still a hack but one option is:

# `tyro.conf.Suppress` hides this field from the generated `ns-render` CLI.
_kill_flag: tyro.conf.Suppress[List[bool]] = field(default_factory=lambda: [False])

@@ -442,6 +470,8 @@ class RenderCameraPath(BaseRender):
"""Filename of the camera path to render."""
output_format: Literal["images", "video"] = "video"
"""How to save output data."""
complete: bool = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with this, it seems like complete should be a @property that gets managed internally

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not really following the connection to @property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can hack around this too by suppressing? I think as a field it does need to still exist for the progress(notification) update.

@kerrj
Copy link
Collaborator

kerrj commented Sep 5, 2024

Some observations from testing it out:

  • If you load a path the render button doesn't become available to click until you add another keyframe.
  • If you click render before 2000 steps, there's no error message except in the terminal (probably shouldn't give a rendering notification unless the render has successfuly started)

@ginazhouhuiwu
Copy link
Contributor Author

Ok sg thanks for testing! I can probably add some error messages in the ui

@kerrj
Copy link
Collaborator

kerrj commented Oct 21, 2024

How is this going? would be good to merge soon so it doesn't go forgotten about

@ginazhouhuiwu
Copy link
Contributor Author

Fixed:

  • error message when user clicks render/export before 2000 training steps
  • able to directly load existing path for rendering
Screenshot 2024-11-06 at 3 58 59 PM

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.

4 participants