Repository navigation
Fix packed covSplats accumulator throwing "Generator must be provided" - #471
jesusluque wants to merge 2 commits into
Conversation
…ded" With covSplats and without extSplats, a stray check after the packed covariance branch required a GsplatGenerator, so a covGenerator alone (which that branch already handles) always threw. Remove it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JtbHkmUpoNeo1EwbHiLihG
| return dynoBlock({ index: "int" }, { covsplat: CovSplat }, () => ({ | ||
| covsplat: new Dyno({ | ||
| outTypes: { covsplat: CovSplat }, | ||
| statements: ({ outputs }) => [ | ||
| `${outputs.covsplat}.flags = GSPLAT_FLAG_ACTIVE;`, | ||
| ], | ||
| }).outputs.covsplat, | ||
| })); |
There was a problem hiding this comment.
For the context of this test, it only needs to have the right shape, so giving the inTypes and outTypes without actually constructing anything suffices:
| return dynoBlock({ index: "int" }, { covsplat: CovSplat }, () => ({ | |
| covsplat: new Dyno({ | |
| outTypes: { covsplat: CovSplat }, | |
| statements: ({ outputs }) => [ | |
| `${outputs.covsplat}.flags = GSPLAT_FLAG_ACTIVE;`, | |
| ], | |
| }).outputs.covsplat, | |
| })); | |
| return dynoBlock({ index: "int" }, { covsplat: CovSplat }, () => ({})); |
| // The packed covariance accumulator (covSplats without extSplats) builds its | ||
| // program from a covGenerator alone, as the extended one does. | ||
| test("packed covSplats accumulator accepts a covGenerator alone", () => { |
There was a problem hiding this comment.
The test description and accompanying comment focus on the "packed" case, yet effectively test both. Could make it a parametrized test (test.each) and generically test that the accumulator accepts just a covGenerator if covSplats is true.
|
Thanks for the contribution. The fix looks good. Would you be able to apply @mrxz's two suggestions on the test, or would you prefer that we take over the PR? |
|
As you wish! Feel free to take over the PR. Thanks! |
The test ran the packed and the extended accumulator inside one case named after the packed one. Each mode is now its own case, and the generator declares only its input and output types.
|
Took this over as discussed, thanks @jesusluque. Applied @mrxz's two suggestions on the test. |
|
@dmarcos This should be ready to merge. |
3 participants
With
covSplats: trueand the packed accumulator (noaccumExtSplats),SplatAccumulatorthrows"Generator must be provided"for every covariance generator.After the packed covariance branch, which already handles a
covGeneratoron its own, a leftover check still required aGsplatGenerator. This PR removes it. Packed covariance splats now generate like the ext ones.Test:
test/unit/SplatAccumulator.test.tsbuilds the packed covariance accumulator for a covariance-only generator. It fails before the fix.🤖 Generated with Claude Code
https://claude.ai/code/session_01JtbHkmUpoNeo1EwbHiLihG