Further improve clamping #4497
Further improve clamping #4497
Conversation
Turns out that billboard and label clamping were fundamentally broken because the `QuadtreePrimitive` was processing the tile queue in the wrong order. It was always pulling new tiles from the back of the array rather than the front, which meant that data would get processed in the wrong order causing old tiles to take precedence over newer tiles. Addtiionally, there was a bad if block in `Label.js` which caused the initial position of the individual label billboards to not be properly set when clamping was on, instead we should always set the positions before calling `_updateClamping` (if needed). Fixes #4396 and #4062
Previously, we would only clamp a billboard or label at it's highest terrain level, we wouldn't "go backwards". This was thought to be a performance benefit. However, in practice it makes it really easy for clamping to be incorrect and you end up with billboards either constantly floating or below ground. This change removes that further optimizaiton and as far as I can tell, clamping is now always correct. The downside of this is that we are doing a big more work and clamping was already slow. However, since most of the time clamping is doing nothing and then bursts, upping the time slice improved performance without any major degradation in frame rate. Once we further optimize the terrain picking code (which is why clamping is so slow to begin with), we could probably further reduce the timeslice.
|
FYI, here's what the profile looks like when lots of data needs to be clamped:
|
|
15ms is a lifetime. I don't want to merge this in haste; let's wait until we can discuss with @bagnell. |
|
Can we retarget this to master so we can merge #4493? |
Yes, I agree. However most of the time we actually spend 0ms doing anything so we only use up the 15ms under load and the temporary drop in frame rate doesn't degrade interactivity. Long term the unified time slicing would obviously just use whatever time remains in the target frame rate.
Yep, that's why I did a separate PR.
Done. |
|
I can still reproduce the bug in this branch. |
|
As @bagnell mentioned, this appears not to fix the problem complete. Looking at the code further and discussing it with him, the optimization is probably correct anyway, so we currently have no idea why certain points still end up floating or buried. We'll have to revisit the issue as soon as we have time. |
Note this is a PR into #4493 You might want to add
?w=1to the url because this is a minor change that looks like more because of indentation.Previously, we would only clamp a billboard or label at it's highest terrain level, we wouldn't "go backwards". This was thought to be a performance benefit. However, in practice it makes it really easy
for clamping to be incorrect and you end up with billboards either constantly floating or below ground. This change removes that further optimizaiton and as far as I can tell, clamping is now always correct.
The downside of this is that we are doing a big more work and clamping was already slow. However, since most of the time clamping is doing nothing and then bursts, upping the time slice improved performance without any major degradation in frame rate. Once we further optimize the terrain
picking code (which is why clamping is so slow to begin with), we could probably further reduce the timeslice.