-
Notifications
You must be signed in to change notification settings - Fork 1.4k
allow saving vertex normal in save_obj #1511
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
Conversation
pytorch3d/io/obj_io.py
Outdated
@@ -728,6 +741,7 @@ def save_obj( | |||
if path_manager is None: | |||
path_manager = PathManager() | |||
|
|||
save_normals = all([n is not None for n in [verts_normals, faces_normals]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea that, if they want, the user can supply verts_normals or faces_normals or both? I think so, and I think the logic for dealing with verts_normals and the logic for dealing with faces_normals should have nothing to do with each other. There's no need for a save_normals
flag which makes them interact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, the logic is that both verts_normal
and faces_normal
are required to save normals in the OBJ file. faces_normal
are used to index the verts_normal
, and for the case when each vertex has only one corresponding normal, it should be the same with faces
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, I misunderstood completely. Could the inputs be called normals
and faces_normals_idx
instead of verts_normals
and faces_normals
? I think it would be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I've changed it. I took the name similar to verts_uvs
and faces_uvs
, but it is indeed a bit misleading.
tests/test_io_obj.py
Outdated
[[0.02, 0.5, 0.73], [0.3, 0.03, 0.361], [0.32, 0.12, 0.47], [0.36, 0.17, 0.9]], | ||
dtype=torch.float32, | ||
) | ||
faces_normals = faces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps faces_normals = faces * 3
or something to make it more obvious that the format is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the test with more normals per face
fd992b1
to
653e498
Compare
26b1e36
to
b18836b
Compare
@bottler has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Although we can load per-vertex normals in
load_obj
, saving per-vertex normals is not supported insave_obj
.This patch fixes this by allowing passing per-vertex normal data in
save_obj
: