Making the Integer Arithmetic more robust when calculating the split statistics for the KD tree #24
Conversation
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
…ginning more gracefully. Adding an initialized flag to each Region. The Region content is now only validated if the region was initialized first.
…or not and if they use integer arithmetic
… using integer arithmetic This bug led to splits with regions containing zero number of samples
…her as shared or static library
…from scratch from multiple sample sets
…se integer arithmetic when parallel pivot splitting is disabled.
… increase robustness on very large scenes and adding some additional checks
…e IntegerSampleStatistics
…able inc variance estimation
…rithmetric based sample statistic estimation
…considers zero valued variance estimates
The regionPivot is separated from the SampleStatistics mean position in case no samples are collected after a spatial split which clears the SampleStatistics.
stefanatwork
approved these changes
Jun 18, 2025
| float minDistance = length(sampleVariance); | ||
| float norm = sampleVariance.x * sampleVariance.x + sampleVariance.y * sampleVariance.y + sampleVariance.z * sampleVariance.z; | ||
| norm = std::max(FLT_EPSILON, norm); | ||
| float minDistance = std::sqrt(norm); |
Collaborator
There was a problem hiding this comment.
Mixing std::sqrt() and sqrt()
| // merge split handling | ||
| regionAndRangeData.first.sampleStatistics.split(splitDim, splitPos, 0.25f, false); | ||
| regionAndRangeDataRight.first.sampleStatistics.split(splitDim, splitPos, 0.25f, true); | ||
| // regionAndRangeData.first.sampleStatistics.split(splitDim, splitPos, 0.25f, false); |
Collaborator
There was a problem hiding this comment.
Is this commented code still necessary or can it be removed?
Collaborator
Author
There was a problem hiding this comment.
I am currently keeping it to remember that I removed it for this fix.
Just in case that we need to revert this.
Maybe adding a comment would be a good idea.
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.
This PR improves some numerical stabilities when using integer arithmetic when calculating the splitting statistics during KD tree building.
In addition, it fixes some numerical instabilities for the VMM fitting when only one sample is used to fit an VMM.
This is an unusual case but now it is at least stable.