Skip to content

Gaussian Splats Writing - #1572

Open
ericmehl wants to merge 2 commits into
ImageEngine:RB-10.7from
ericmehl:gSplatsWrite
Open

ericmehl wants to merge 2 commits into
ImageEngine:RB-10.7from
ericmehl:gSplatsWrite

Conversation

@ericmehl

Copy link
Copy Markdown
Collaborator

This adds write support to USD for ParticleField3DGaussianSplat objects.

It also changes the reading of ParticleField3DGaussianSplat slightly. Previously, we were only loading radiance:sphericalHarmonicsCoefficients that the radiance:sphericalHarmonicsDegree indicated were used. That meant you would not load data if the degree indicated fewer coefficients were used than what were present( # coefficients = ( degree + 1 ) * ( degree + 1 ) ).
Now we're treating the degree and the coefficients as more-or-less independent quantities. They are related though, so I'm warning if they don't agree and continue loading.

Writing has some similar edge cases that might be worth more discussion. Since we're collecting the ParticleField3DGaussianSplat:# primvars into a single vector, some decisions need to be made if the data isn't formatted correctly. My goal is to keep as much data as possible to let the user make fixes if needed. I believe the only data that will be lost from this code is if a coefficients primvar has a valid index (like ParticleField3DGaussianSplat:15) but non-V3fVectorData type or non-Vertex interpolation. The fallback in that case is ideally to write the data out as a separate variable, but to USD the :# suffix gets interpreted as a namespace with a number as the name and it is warned as being invalid.

Perhaps we should not emit our own warning and let USD complain? The ParticleField3DGaussianSplat:# format is something of our making, so my thought is we should be the ones making the warning.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Cortex project's prevailing coding style and conventions.

Previously we were only reading the number of coefficients that
`radiance:sphericalHarmonicsDegree` indicated we should get. In order to
preserve data, we now load all of the coefficients possible.

@johnhaddon johnhaddon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Eric! LGTM, with one simple suggestion for strengthening the tests. Could you fix that while squashing in your fixup commit please, and then I think we're good to go...

Comment on lines +5286 to +5296
self.assertEqual( readPoints["type"].data, typeData )
self.assertEqual( readPoints["P"].data, positionsData )
self.assertEqual( readPoints["orientations"].data, orientationsData )
self.assertEqual( readPoints["scales"].data, scalesData )
self.assertEqual( readPoints["opacities"].data, opacitiesData )
self.assertEqual( readPoints["projectionModeHint"].data, projectionModeHintData )
self.assertEqual( readPoints["sortingModeHint"].data, sortingModeHintData )
self.assertEqual( readPoints["radiance:sphericalHarmonicsDegree"].data, degreeData )
for i in range( 0, nCoefficients ) :
self.assertEqual( readPoints[f"radiance:sphericalHarmonicsCoefficients:{i}"].data, coefficientsData[i] )
self.assertEqual( readPoints["Cs"].data, displayColorData )

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing each property tested individually made me wonder if something would fail if we did self.assertEqual( readPoints, cortexSplats ). It seems that passes though, so since it is a stronger test I think it's worth doing that instead.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and squashed into ca72db1

@ericmehl

Copy link
Copy Markdown
Collaborator Author

Thanks @johnhaddon! I firmed up that test and squashed everything down. I'll wait to push the merge button, but it should be ready to go.

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

Successfully merging this pull request may close these issues.

2 participants