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 PixelSampler the depth-nerfacto default #3454

Conversation

nepfaff
Copy link
Contributor

@nepfaff nepfaff commented Sep 28, 2024

The default pixel sampler for depth-nerfacto is PairPixelSampler. However, this is very slow due to mask erosion (so slow that it is basically unusable). I managed to speed it up a little here #3452. However, it is still much more efficient to use PixelSampler. Moreover, PairPixelSampler is only needed when using SPARSENERF_RANKING as the depth loss, but the default value is DS_NERF, which doesn't require PairPixelSampler. Hence, with the default depth loss, most people will be much better off with PixelSampler instead of PairPixelSampler.

I tested with both depth and masks using 50 images of resolution 1920x1440. Note, that PairPixelSampler is only slow when training with masks due to the need for mask erosion.

With PairPixelSampler:
image

With PixelSampler:
image

Closes #3446

@akristoffersen akristoffersen self-requested a review October 1, 2024 06:12
Copy link
Contributor

@akristoffersen akristoffersen left a comment

Choose a reason for hiding this comment

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

This makes sense to me. Approving.

It's a bit unfortunate that there's no obvious sign to the user that PairPixelSampler should be used when using SPARSENERF_RANKING. I believe it would silently fail if that depth loss was used without the pair pixel sampler. Could you add to the comment under the loss type in DepthNerfactoModelConfig to note this?

@nepfaff nepfaff force-pushed the make_pixel_sampler_depth_nerfacto_default branch from 55a517e to 3db8e4e Compare October 1, 2024 09:06
@nepfaff
Copy link
Contributor Author

nepfaff commented Oct 1, 2024

Added a comment. It would be best to throw an exception in DepthNerfactoModel but that doesn't seem to be possible with the current structure.

@akristoffersen akristoffersen enabled auto-merge (squash) October 1, 2024 16:55
@akristoffersen akristoffersen force-pushed the make_pixel_sampler_depth_nerfacto_default branch from 3db8e4e to ee857f2 Compare October 1, 2024 16:56
@akristoffersen akristoffersen merged commit 334a1c8 into nerfstudio-project:main Oct 1, 2024
3 checks passed
@nepfaff nepfaff deleted the make_pixel_sampler_depth_nerfacto_default branch October 2, 2024 09:30
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.

Depth-Nerfacto + Masking = Very slow
2 participants