Refactor Gaussian splatting ops and extract utility functions#596
Conversation
c2ba2a6 to
56fd532
Compare
|
I think we also have some Python function naming leaking into the C++ side. For example, |
It's intentional. I'm trying to have the operator names and the binding names align. |
Sorry I mean the conventions -- In the C++ code, it should be named |
If we move all 3DGS ops into the top-level ops directory, it would be helpful to start all 3DGS files with a common prefix. Previously we had "Gaussian". |
They are standalone ops now with a consistent naming that matches their python counterpart names. e.g. I'd push back a bit on adding What are your thoughts? |
Why do we want to move the Gaussian-related ops out of |
For the same reason we haven't split up GridBatch and all the JaggedTensor ops into their own folders. If we do this, we can segment out different ops into different directories based on their type but I think its not always a clean mapping. E.g. here spherical harmonics evaluation is a generic operator that we happen to use a lot in conjunction with Gaussian splatting. We can certainly try and group .cu/.h pairs into folders for code organization purposes but I wonder if we shouldn't do this all in one go. Personally, I prefer to have all the ops be in one big folder but I don't feel that strongly one way or another. If we're going to partition op files, I prefer to do it consistently. |
But the reality is that there is no functionality that uses the spherical harmonics evaluation operators outside of Gaussian splatting and the arguments to the SH evaluation ops are specific to the Gaussian splatting toolchain (e.g. the means and projected radii arguments). Until there's a reason to move them into a more generic folder, I don't see why we would change that from where they stand today. |
|
Unless it's a public interface that we want to simplify #include lines for, I prefer to have things more organized rather than one big dump folder. |
Sounds good - I moved it into a folder for better organization. Let me know if you guys are happy with the PR otherwise! |
swahtz
left a comment
There was a problem hiding this comment.
Loving the removal of the templated dispatch functions everywhere. Just a few docs/naming and other improvement comments. Thanks
|
If we're going to keep the splatting operators in a subdirectory, can we just keep the gsplat folder name instead of introducing a new gaussian_splatting folder? It would make the changes easier to follow through the history in the short term, and we can rename it in a straightforward follow up PR. |
|
Done -- renamed back to |
- Add input validation on sh0 shape in evaluate_spherical_harmonics - Clean up GaussianSplatOps.cpp comments (remove PR-specific notes, empty sections, parentheticals, numbered function comments) - Use empty tensor for shN at degree 0 in GaussianSplat3d._eval_sh to avoid holding the full tensor in the autograd graph - Revert prepare_raster_optional_inputs to camelCase (not Python-bound) - Add doxygen @param/@return docs to AffineTransform.cuh, GaussianMath.cuh, and GaussianRasterizeFromWorld.cuh helpers - Rename ops/gaussian_splatting/ back to ops/gsplat/ per reviewer request - Update _fvdb_cpp.pyi stub to match current bindings (add 16 missing function stubs, remove 2 stale ones, fix signatures) Signed-off-by: Francis Williams <francis@fwilliams.info> Made-with: Cursor
matthewdcong
left a comment
There was a problem hiding this comment.
Sorry for the continued comments. I have some more feedback after checking out your branch and playing around with it.
3cc3bf8 to
ea849b7
Compare
swahtz
left a comment
There was a problem hiding this comment.
Looking good, thanks for taking on board my notes and explaining the changes.
|
Can we update the PR and title description to reflect the latest since it ends up going into the final squashed MR? Also, please add that |
9b09c93 to
e6f34aa
Compare
|
Modified the description and title. Lmk if anything is off |
e6f34aa to
ffd7623
Compare
This PR is primarily code migration and renaming for consistency. It introduces no functional changes and removes some code duplication. What is included: - Renames 21 Gaussian splatting operator file pairs in `ops/gsplat/` with descriptive names (e.g. `GaussianProjectionForward` -> `ProjectGaussiansAnalyticForward`, `GaussianRasterizeForward` -> `RasterizeScreenSpaceGaussiansForward`) - Extracts shared CUDA utilities into `utils/cuda/` (BinSearch, CubWrapper, Prefetch, WarpReduce, Alignment, CopyCoords, OpType) and `utils/cuda/math/` (AffineTransform, Rotation) - Extracts Gaussian-specific utilities into `utils/gsplat/` (Gaussian2D, GaussianCameraAccessorCopy, GaussianCameras, GaussianMath, GaussianRasterize, GaussianRasterizeFromWorld, GaussianRasterizeOptionalInputs) - Removes the template dispatch pattern (`dispatchFoo<DeviceTag>`) from op headers in favor of plain functions with internal device dispatch - Deletes dead gsplat files: GaussianUtils.cpp/.h/.cuh, GaussianVectorTypes, GaussianMacros, GaussianRenderSettings, GaussianRigidTransform, GaussianCameraMatrixUtils, GaussianWarpUtils - Renames `GaussianSplatBinding.cpp` -> `GaussianSplatOps.cpp` - Updates all include paths in bindings, viewer, PLY I/O, and 16 C++ test files - Removes `OpType` struct in favor of PyTorch's built-in `at::opmath_type` - Removes `GSPLAT_PRAGMA_UNROLL` struct in favor of `#pragma unroll` Part of #590 (PR 2: refactor gsplat directory). Co-authored-by: Matthew Cong <mcong@nvidia.com> Made-with: Cursor
ffd7623 to
878cf73
Compare
This PR is primarily code migration and renaming for consistency. It introduces no functional changes and removes some code duplication. What is included: - Renames 21 Gaussian splatting operator file pairs in `ops/gsplat/` with descriptive names (e.g. `GaussianProjectionForward` -> `ProjectGaussiansAnalyticForward`, `GaussianRasterizeForward` -> `RasterizeScreenSpaceGaussiansForward`) - Extracts shared CUDA utilities into `utils/cuda/` (BinSearch, CubWrapper, Prefetch, WarpReduce, Alignment, CopyCoords, OpType) and `utils/cuda/math/` (AffineTransform, Rotation) - Extracts Gaussian-specific utilities into `utils/gsplat/` (Gaussian2D, GaussianCameraAccessorCopy, GaussianCameras, GaussianMath, GaussianRasterize, GaussianRasterizeFromWorld, GaussianRasterizeOptionalInputs) - Removes the template dispatch pattern (`dispatchFoo<DeviceTag>`) from op headers in favor of plain functions with internal device dispatch - Deletes dead gsplat files: GaussianUtils.cpp/.h/.cuh, GaussianVectorTypes, GaussianMacros, GaussianRenderSettings, GaussianRigidTransform, GaussianCameraMatrixUtils, GaussianWarpUtils - Renames `GaussianSplatBinding.cpp` -> `GaussianSplatOps.cpp` - Updates all include paths in bindings, viewer, PLY I/O, and 16 C++ test files - Removes `OpType` struct in favor of PyTorch's built-in `at::opmath_type` - Removes `GSPLAT_PRAGMA_UNROLL` struct in favor of `#pragma unroll` Part of #590 (PR 2: refactor gsplat directory). Co-authored-by: Matthew Cong <mcong@nvidia.com> Signed-off-by: Francis Williams <francis@fwilliams.info> Signed-off-by: Matthew Cong <mcong@nvidia.com> Made-with: Cursor
878cf73 to
ad90271
Compare
Summary
This PR is primarily code migration and renaming for consistency. It introduces no functional changes and removes some code duplication.
What is included:
ops/gsplat/with descriptive names (e.g.GaussianProjectionForward->ProjectGaussiansAnalyticForward,GaussianRasterizeForward->RasterizeScreenSpaceGaussiansForward)utils/cuda/(BinSearch, CubWrapper, Prefetch, WarpReduce, Alignment, CopyCoords, OpType) andutils/cuda/math/(AffineTransform, Rotation)utils/gsplat/(Gaussian2D, GaussianCameraAccessorCopy, GaussianCameras, GaussianMath, GaussianRasterize, GaussianRasterizeFromWorld, GaussianRasterizeOptionalInputs)dispatchFoo<DeviceTag>) from op headers in favor of plain functions with internal device dispatchGaussianSplatBinding.cpp->GaussianSplatOps.cppOpTypestruct in favor of PyTorch's built-inat::opmath_typeGSPLAT_PRAGMA_UNROLLstruct in favor of#pragma unrollPart of #590 (PR 2: refactor gsplat directory).
Test plan
GaussianRasterizeForwardMaskedEdgeTile.NoDeadlockdeadlock timeout,PredGatherIGemmTestCUDA launch failure)cd tests && pytest unit -v)