pytorch3d
pytorch3d copied to clipboard
facebookresearch
Pulsar unified interface update
This commit addresses several braking changes to PyTorch3d camera conventions that made the unified system unusable at the moment (see https://github.com/facebookresearch/pytorch3d/issues/1352 and https://github.com/facebookresearch/pytorch3d/issues/1276). This commit fixes all these problems and tests using the test case from https://github.com/facebookresearch/pytorch3d/issues/1352. The results are equivalent for FoVOrthographicCameras, OrthographicCameras, FoVPerspectiveCameras and PerspectiveCameras.
We used to have a test case for the unified interface for Pulsar that compared the rendering results with some 'ground truth' images. This test seems to have been removed, thus leading to these breaking changes remaining undiscovered for a long time. Would be great if we could revive that test case!
Hi @classner!
Thank you for your pull request.
We require contributors to sign our Contributor License Agreement, and yours needs attention.
You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
Any updates on this PR?
I'm getting "Pulsar only supports a single focal length! " And I hope this PR can fix it
Hi @chenyuntc , @nlml - I have left Meta since opening this PR, maybe @bottler can have a look?
@classner Hi! Sorry I think it would be easiest if you do actually fill in the CLA again. I can work around it though if you don't want to.
@bottler has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Hi.
I made 2 easy updates to this locally for some test problems:
- Instances of
focal_length_conf *= ...I replaced with out-of-placefocal_length_conf = focal_length_conf * ...because there are cases where focal_length_conf is an expanded tensor. - New versions of
tests/data/test_pulsar_simple_pointcloud_sphere_azimuth{90,0}.0_fovperspective.pngbecause the points are slightly different sizes.
I think I can push these back to your branch if you want.
However I have a problem with test_camera_pixels.py. It tries to check that all the renderers are pixel-perfect in their interpretation of PerspectiveCameras, the most common camera object. This change breaks that test for the camera in ndc. I think that this code is assuming that width is greater than height, and that the fix is to replace width and height with max(width,height) and min(width, height) respectively in all the new code in this change. Do you agree? More generally, I think I'd expect this code to be symmetrical in width and height.
Hmmm, in the renderer there is no assumption about the larger dimension in there - possibly in this preprocessing part? Worth a try looking at these scaling operations - they are mainly there to make Pulsar work well with the assumptions made in the PyTorch3D cameras, so you should have a better understanding than me about what those are at the various points. I might have snuck in a mistake there inadvertently.