-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Open for discussion] Rework/remove density_fn
to support Gaussians.
#2450
base: main
Are you sure you want to change the base?
Conversation
Does anybody have a sense for how many (public) external repositories would be impacted by this? |
I believe external repos should fix an NS version. The development breaks lots of stuff between versions so I think its fine. Perhaps keeping a deprecation warning for a couple of versions would be nice. |
Do you agree to merge this as is, or should we add some deprecation warnings? |
This seems like a reasonable thing to recommend!
I skimmed a few repos, and at the very least it seems like the K-Planes extension will break from this PR: That said, I'm okay with merging without deprecation warnings. If we do this we can follow up with an official recommendation for external methods to pin a nerfstudio version/commit in the docs, and perhaps make PRs for fixing versions of external methods that we know are out there? Does this make sense? cc also @kerrj @tancik |
So anyone willing to make a decision on this? |
@brentyi , do you agree with merging? |
Yes, this has my support! For moving forward, how about:
Any opposition @Mxbonn @jkulhanek? If not I'm also happy to help with some of this. |
I agree. I will add the error message soon. |
issue
The
density_fn
inclass Field
takes different arguments thanget_density
(it takes positions instead of RaySamples).As a result, it is not possible to use the ProposalNetworkSampler with networks that do not take positions as arguments. One example is: MipNerf360, which uses Gaussians in the proposal network.
More generally, I don't think it makes sense to have a function that is nearly identical to
get_density
, but that takes in non-standard arguments.Proposed fix
remove/deprecate
density_fn
. It can nearly always be replaced withget_density
. In places where it is not straightforward to replace it withget_density
(e.g. the occupancy grid) use a custom wrapper function.Problem with the fix
It is a breaking change, but a breaking change will be necessary if you want to support e.g. mipnerf360.
I don't know the policy for breaking changes, but one could always not immediately remove the
density_fn
and put a deprecation warning on it.