facebookresearch / facebookresearch/pytorch3d
Errors in NeRF implementation
- Dominant language
- Python
- Stars
- 10k
- Forks
- 1.5k
- PR merge metrics
- No merged PRs in 30d
Description
Hello, first of all, thanks for the beautiful implementation!
However, certain things seem to be buggy. Please correct me if I am wrong:
### Problem 1.
`NDCGridRaysampler` does not work properly with rectangular images. Since the NDC convention assumes that the XY coordinates lie in `[-1, 1] x [u, u]` or (`[-u, u] x [-1, 1]`) depending on which size is shorter.
The simple fix I expect would be:
```
from pytorch3d.renderer.mesh.rasterize_meshes import pix_to_non_square_ndc
min_x = pix_to_non_square_ndc(image_width - 1, image_width, image_height)
max_x = pix_to_non_square_ndc(0, image_width, image_height)
min_y = pix_to_non_square_ndc(image_height - 1, image_height, image_width)
max_y = pix_to_non_square_ndc(0, image_height, image_width)
```
A similar problem also persists here when passing arguments:
https://github.com/facebookresearch/pytorch3d/blob/9585a58d10cb2efcd159b058fa4af914203c1d0d/projects/nerf/nerf/raysampler.py#L162-L166
If the fix is applied, then grid_sample should be fixed to account for it (the longer side should be divided by `u`)
https://github.com/facebookresearch/pytorch3d/blob/9585a58d10cb2efcd159b058fa4af914203c1d0d/projects/nerf/nerf/utils.py#L51
also, `align_corners` should be `False` since the in NDC convention, the pixel location corresponds to its center
https://github.com/facebookresearch/pytorch3d/blob/6d36c1e2b00d63d994fd4dd7d0b740f1922443df/projects/nerf/nerf/utils.py#L53-L58
### Problem 2.
deltas in `_get_densities` do not take into account the norm of `ray_directions`
https://github.com/facebookresearch/pytorch3d/blob/9585a58d10cb2efcd159b058fa4af914203c1d0d/projects/nerf/nerf/implicit_function.py#L109-L115
this does not lead to error since the `ray_directions` were normalized in `NeRFRaysampler`
but this leads to another problem: since the `ray_directions` were normalized, `ray_bundle_to_ray_points` will now produce points that lie before the near plane if, e.g., I pass ray_lengths = near (so the depth values are not actually depths)
it seems to me it is better to avoid `ray_directions` normalization and take into account the norm of `ray_directions` when calculating densities like it is done in an original implementation https://github.com/bmild/nerf/blob/20a91e764a28816ee2234fcadb73bd59a613a44c/run_nerf.py#L123
### Problem 3.
NeRFRaysampler does not correctly work with batch_size > 1
e.g., here
https://github.com/facebookresearch/pytorch3d/blob/8fa438cbda382602ad64afac5713f4e7e0461f88/projects/nerf/nerf/raysampler.py#L349-L357
causes the case when the rays originally pointing to the different batch idx will now point to the same batch_idx
also, some index bound errors would be here:
https://github.com/facebookresearch/pytorch3d/blob/8fa438cbda382602ad64afac5713f4e7e0461f88/projects/nerf/nerf/raysampler.py#L329
https://github.com/facebookresearch/pytorch3d/blob/8fa438cbda382602ad64afac5713f4e7e0461f88/projects/nerf/nerf/raysampler.py#L338-L347
here, e.g., also no batch dimension in data actually going to the function
https://github.com/facebookresearch/pytorch3d/blob/8fa438cbda382602ad64afac5713f4e7e0461f88/projects/nerf/nerf/nerf_renderer.py#L293-L294
https://github.com/facebookresearch/pytorch3d/blob/8fa438cbda382602ad64afac5713f4e7e0461f88/projects/nerf/nerf/nerf_renderer.py#L210-L211
Contributor guide
Assessment
This issue has not been assessed yet.