Fix Training Crop Box Pruning#1365
Open
kmeirlaen wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes live training crop-box pruning when the crop box is transformed (by using a world-to-box transform) and prevents intermittent Vulkan viewport failures during crop-driven topology compaction by extending renderer exclusion to cover crop-prune tensor reallocation and subsequent storage repair.
Changes:
- Extracted crop-box removal mask computation into
src/training/training_cropbox_mask.*and updated the trainer to use the shared helper. - Corrected crop-box transform usage for pruning by applying
glm::inverse(scene.getWorldTransform(cropbox_id))(world-to-cropbox). - Added focused regression tests for translated crop boxes, inverse masks, soft-deleted splats, and nullopt/no-op cases; wired the new test into the tests CMake list.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/training/trainer.cpp |
Uses shared crop-box mask helper and expands renderer exclusion locking to cover crop-prune topology changes and Vulkan-external storage repair. |
src/training/training_cropbox_mask.hpp |
Declares shared crop-box remove-mask helpers for production and test use. |
src/training/training_cropbox_mask.cpp |
Implements the crop-box remove-mask logic, including correct world-to-cropbox transform handling. |
tests/test_training_cropbox_mask.cpp |
Adds regression coverage for transformed crop pruning semantics and scene/model non-mutation contracts. |
src/training/CMakeLists.txt |
Adds the new helper implementation file to the training sources. |
tests/CMakeLists.txt |
Adds the new regression test to the test target sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes live training crop-box pruning for transformed crop boxes and prevents intermittent Vulkan viewport failures while crop pruning changes model topology.
The crop-box mask logic has been extracted from
trainer.cppinto a shared training helper so the trainer and regression tests exercise the same implementation.Problem
Live training supports an enabled crop box attached to the training model and continuously prunes gaussians that fall on the removed side of that box. That pruning path was using the crop box node's world transform directly as the transform for the inside/outside test.
Scene node transforms are box-to-world transforms. The crop test needs the opposite direction: world-to-box. When the crop box was translated, rotated, or scaled, training evaluated gaussian positions in the wrong coordinate frame. The practical result was that valid splats inside the visible crop box could be removed while splats outside the crop boundary survived. In one test with a fitted/trimmed crop box, this could remove all initial gaussians and fail training with
n_primitives is 0 - model has no gaussians.Fixing the crop transform exposed a second issue. Once the crop mask was correct, live training could prune small numbers of gaussians repeatedly during ordinary non-refinement iterations.
remove_gaussians()compacts model tensors, and the GUI/Vulkan training path then repairs those tensors back into Vulkan-external storage. The trainer only excluded the renderer during strategy refinement, so a viewport frame could sometimes observe the model after compaction but before Vulkan-external storage was restored.When that happened, VkSplat intentionally refused the unsafe full input-copy fallback and logged errors such as:
The user-facing symptom was an intermittent black viewport during training. Moving the camera or otherwise triggering a later redraw could make the splats appear again after the storage repair completed.
Reproduction Steps
Wrong crop pruning
Fit to Scene (Trimmed)so the crop box transform is not identitcal.n_primitives is 0 - model has no gaussiansVulkan black viewport during crop pruning
Note: this could only be replicated if first problem is resolved, so without the PR this is not possible to simulate, and with PR this is resolved)
Use a crop box that removes only a small number of gaussians during live training.
Start training with the Vulkan viewport active.
Watch the training log while crop pruning runs repeatedly.
Before this fix, logs could show
MRNF::remove_gaussiansfollowed by:The viewport could temporarily go black even though training continued.
Moving the camera or otherwise forcing a later redraw could make the splats appear again.
What Changed
glm::inverse(scene.getWorldTransform(cropbox_id)), matching the world-to-box convention used by crop-box filtering.src/training/training_cropbox_mask.*.compute_training_cropbox_remove_mask(...)is still production code used by the trainer.compute_cropbox_remove_mask(...)covers the pure tensor/math portion for direct testing.Files
src/training/trainer.cppsrc/training/training_cropbox_mask.hppsrc/training/training_cropbox_mask.cpptests/test_training_cropbox_mask.cppsrc/training/CMakeLists.txttests/CMakeLists.txtTesting
Manual Validation
VkSplat refusing full input-copy fallbackduring crop-prune iterations.Risk
trainer.cpp.