facebookresearch / facebookresearch/pytorch3d

Errors in NeRF implementation

Open
#868 12 comments 1 reaction 1 assignee Claimed by @davnov134 View on GitHub
do-not-reap how to
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.