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

Render cameras with render times for dynamic nerfs #1199

Merged
merged 5 commits into from
Jan 4, 2023
Merged

Render cameras with render times for dynamic nerfs #1199

merged 5 commits into from
Jan 4, 2023

Conversation

nlml
Copy link
Contributor

@nlml nlml commented Jan 2, 2023

See #1178

Add a --dynamic flag to ns-render. If this is provided, the get_path_from_json will look for the field render_time in each camera of the camera_path (which comes from UI-exported .json file). If these fields are present, then when constructing the Cameras, these render_times are used. This allows for varying timestamps when rendering from a dynamic nerf.

On the viewer side, like with the "Render Timestep" control on the Controls tab, if the /model/has_temporal_distortion msg is true, then controls for the Render cameras' timesteps will be visible. Using these additional controls, one can now set the render_times that are exported to the camera path .json. This is acheived basically by copying the setup for FOV. Like FOV, the render_time can either be the same for all cameras, or specified per-camera. As per FOV, smoothness is applied to the per-camera render_times.

Have tested this PR on a private dataset and it works as expected. If needed I could put together a toy dataset as a test.

Potential (minor) issues or nitpicks with this PR:

  • Depending on the smoothness and the input values, the exported render_times can be <0.0 or >1.0. This is probably not a huge deal and the exported values shouldn't go too far beyond this range, but perhaps we want to clip the values.
  • Just a visual nitpick: the Render Time displayed in the viewer UI will show '1' or '0' instead of '1.0' and '0.0' if the entered value is exactly 1 or 0.
  • I believe also now all exported camera_path .jsons will contain this render_time field, even if the path was exported from a non-dynamic nerf. This shouldn't be a problem however as they will be ignored by ns-render if the --dynamic flag is not passed.

Copy link
Contributor

@tancik tancik left a comment

Choose a reason for hiding this comment

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

Nice!
Can you add the --dynamic to the render command when it is used -

const cmd = `ns-render --load-config ${config_filename} --traj filename --camera-path-filename ${camera_path_filename} --output-path renders/${filename}.mp4`;

Also I think we should clamp the time to 0-1, as some models may require that.

@nlml
Copy link
Contributor Author

nlml commented Jan 3, 2023

Added the --dynamic flag to the render cmd if it is a dnerf, and render times are now clamped to [0,1] before writing to the json

@nlml
Copy link
Contributor Author

nlml commented Jan 3, 2023

Just fixed a bug with clamping in the previous commit - should be good now.

@tancik
Copy link
Contributor

tancik commented Jan 3, 2023

Thinking a little more about it, does it make sense not to include the time in the json if time isn't used. Then the export script can render with time only if it is present? That would remove the requirement for the user to specify --dynamic

@nlml
Copy link
Contributor Author

nlml commented Jan 4, 2023

Thinking a little more about it, does it make sense not to include the time in the json if time isn't used. Then the export script can render with time only if it is present? That would remove the requirement for the user to specify --dynamic

Makes sense, actually I initially thought this too but since it was my first time with React, I was just going with what seemed simplest at the time. But I've changed it now to work as you described and it actually makes the overall PR and changes much simpler.

Thus now in ns-render we only pass times to Cameras if all items in the camera_paths contain the render_time key. And these keys are only exported by the viewer if /model/has_temporal_distortion msg is true.

I re-tested with both a dnerf and nerfacto (non-dynamic) model and the exported .json is either with/without the render_time keys, as expected, so I think we are good.

Copy link
Contributor

@tancik tancik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making those changes. Sorry for not mentioning it earlier.

@tancik tancik merged commit 31be97d into nerfstudio-project:main Jan 4, 2023
@nlml
Copy link
Contributor Author

nlml commented Jan 4, 2023

No worries! Thanks!

tancik pushed a commit to dozeri83/nerfstudio that referenced this pull request Jan 20, 2023
…t#1199)

* Render cameras with render times for dynamic nerfs

* Add --dynamic flag to cmd. Clamp render times.

* Fix for issues with clamping renderTime

* Only add render_time to camera path if dynamic nerf

* change DEFAULT_TIME to DEFAULT_RENDER_TIME

Co-authored-by: Liam Schoneveld <[email protected]>
lucasthahn pushed a commit to tne-ai/nerfstudio that referenced this pull request Jan 26, 2023
…t#1199)

* Render cameras with render times for dynamic nerfs

* Add --dynamic flag to cmd. Clamp render times.

* Fix for issues with clamping renderTime

* Only add render_time to camera path if dynamic nerf

* change DEFAULT_TIME to DEFAULT_RENDER_TIME

Co-authored-by: Liam Schoneveld <[email protected]>
chris838 pushed a commit to chris838/nerfstudio that referenced this pull request Apr 22, 2023
…t#1199)

* Render cameras with render times for dynamic nerfs

* Add --dynamic flag to cmd. Clamp render times.

* Fix for issues with clamping renderTime

* Only add render_time to camera path if dynamic nerf

* change DEFAULT_TIME to DEFAULT_RENDER_TIME

Co-authored-by: Liam Schoneveld <[email protected]>
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