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

Make resetting the near_plane in NearFarCollider optional. #2465

Merged
merged 8 commits into from
Oct 27, 2023

Conversation

Mxbonn
Copy link
Contributor

@Mxbonn Mxbonn commented Sep 24, 2023

It is not always desired to reset the near plane to 0 during inference. Added an optional flag.

@jkulhanek
Copy link
Contributor

I'm curious why it was there in the first place. Could it be removed for good? @tancik

@tancik
Copy link
Contributor

tancik commented Sep 25, 2023

I think it was there because otherwise the scene can get clipped when viewing it in the viewer. iirc this was an issue with the blender synthetic datasets awhile ago.

@jkulhanek
Copy link
Contributor

I wonder if we can remove this for good...

@kerrj
Copy link
Collaborator

kerrj commented Sep 25, 2023

It seems like this would only matter if the camera gets super close to objects in the scene, how often do people do this? What situations would you want to keep the near plane the same as training?

@Mxbonn
Copy link
Contributor Author

Mxbonn commented Sep 25, 2023

It seems like this would only matter if the camera gets super close to objects in the scene, how often do people do this? What situations would you want to keep the near plane the same as training?

Well some methods do not even work when the near plane is zero. E.g. if the sampler is LinearDisparitySampler due to the 1/near_plane. This could also be made to work if you replace 0 with a small epsilon but I thought keeping the initial near_plane much more logical.

@jkulhanek
Copy link
Contributor

It seems like this would only matter if the camera gets super close to objects in the scene, how often do people do this? What situations would you want to keep the near plane the same as training?

I think changing the near plane skews the sampling in nerf/mipnerf potentially decreasing the eval perf.

@jkulhanek
Copy link
Contributor

I would vote for removing resetting to zero. I don’t think having the option here is useful.

@jkulhanek
Copy link
Contributor

@brentyi, do you accept this PR?

@brentyi
Copy link
Collaborator

brentyi commented Oct 27, 2023

It looks good to me, I just tweaked the comment to add details on why we reset the clipping plane.

I don't feel strongly, but I'd vote to keep the argument + current reset behavior. I agree that it'd likely hurt eval metrics when evaluation images are a similar distance to the object as the training images (as is the case in the synthetic datasets), but it would dramatically improve them if the evaluation images get much closer (not present in evaluation datasets, but IMO more useful in the real world).

@brentyi brentyi enabled auto-merge (squash) October 27, 2023 20:46
@brentyi brentyi merged commit 793fca5 into nerfstudio-project:main Oct 27, 2023
4 checks passed
Yosshi999 pushed a commit to Yosshi999/nerfstudio that referenced this pull request Nov 3, 2023
…o-project#2465)

* Make resetting the near_plane in NearFarCollider optional.

It is not always desired to reset the near plane to 0.0 during inference.
Added an optional flag.

* no need to change precision

* Update scene_colliders.py

---------

Co-authored-by: Brent Yi <yibrenth@gmail.com>
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.

None yet

5 participants