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

result from marching_cubes has identical vertex indices #1731

Closed
Khoa-NT opened this issue Feb 13, 2024 · 3 comments
Closed

result from marching_cubes has identical vertex indices #1731

Khoa-NT opened this issue Feb 13, 2024 · 3 comments

Comments

@Khoa-NT
Copy link

Khoa-NT commented Feb 13, 2024

🐛 Bugs / Unexpected behaviors

The result from marching_cubes on GPU is not correct

Instructions To Reproduce the Issue:

pytorch3D installed from commit c292c71
Previous test result is in #1641 (comment)

import torch
from pytorch3d.ops.marching_cubes import marching_cubes, marching_cubes_naive
from pytorch3d.io import save_obj

from skimage import measure
from skimage.draw import ellipsoid

# Generate a level set about zero of two identical ellipsoids in 3D
ellip_base = ellipsoid(50, 60, 16, levelset=True)
ellip_base_pt = torch.from_numpy(ellip_base).unsqueeze(0).float() # (N, D, H, W)


### 1) Run on CPU with marching_cubes_naive
verts_cpu_naive, faces_cpu_naive = marching_cubes_naive(ellip_base_pt, 0)
save_obj('marching_cubes_naive.obj', verts_cpu_naive[0], faces_cpu_naive[0])

### 2) Run on CPU with marching_cubes
verts_cpu, faces_cpu = marching_cubes(ellip_base_pt, 0)
save_obj('marching_cubes_cpu.obj', verts_cpu[0], faces_cpu[0])

### 3) Run on CUDA with marching_cubes
verts_cuda, faces_cuda = marching_cubes(ellip_base_pt.cuda(), 0)
save_obj('marching_cubes_cuda.obj', verts_cuda[0], faces_cuda[0])

View from meshlab

1) Run on CPU with marching_cubes_naive

image

2) Run on CPU with marching_cubes

image

3) Run on CUDA with marching_cubes

image

image

Khoa-NT referenced this issue Feb 13, 2024
Summary:
Fixes #1641. The bug was caused by the mistaken downcasting of an int64_t into int, causing issues only on inputs large enough to have hashes that escaped the bounds of an int32.

Also added a test case for this issue.

Reviewed By: bottler

Differential Revision: D53505370

fbshipit-source-id: 0fdd0efc6d259cc3b0263e7ff3a4ab2c648ec521
@roym899
Copy link

roym899 commented Feb 27, 2024

I have the same problem on 0.7.6; for me 0.7.4 is the last version that works correctly.

facebook-github-bot pushed a commit that referenced this issue Mar 7, 2024
Summary:
Fix an inclusive vs exclusive scan mix-up that was accidentally introduced when removing the Thrust dependency (`Thrust::exclusive_scan`) and reimplementing it using `at::cumsum` (which does an inclusive scan).

This fixes two Github reported issues:

 * #1731
 * #1751

Reviewed By: bottler

Differential Revision: D54605545

fbshipit-source-id: da9e92f3f8a9a35f7b7191428d0b9a9ca03e0d4d
@JaapSuter
Copy link
Contributor

I believe this to be fixed by 7566530, I ran your repro locally and it succeeded. Do let me know if it's still broken for you. Thanks!

@roym899
Copy link

roym899 commented May 3, 2024

I can confirm this is working again on main.

@bottler bottler closed this as completed May 3, 2024
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

No branches or pull requests

4 participants