Revive model picking refactoring #12658
Draft
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.
Description
This essentially revives the changes from #11830 , including some fixes like #11842 (i.e. it is based on the latest state). Some general ideas behind the changes here are described in detail in this linked PR, but I'll try to summarize them and add further thoughts:
The original
pickModel
functionality intersected a picking ray with all triangles of all instances of a model. It did this with pseudocodeThis looks innocent, but isn't:
Avoiding the repeated instance transforms is relatively easy: Instead of transforming the triangle vertices with the instance transform, one can simply intersect the triangle vertices with a ray that was transformed with the inverse instance transform.
An additional, critical point here is that "fetching" a vertex involves several potentially very costly steps. The
pickModel
function checks whether the indices and positions already are on the CPU (as a typed array). If this is not the case, then the data is read from a GPU buffer into a CPU typed array. By default, this is done each time when the function is called (which may be hundreds of times per frame!). This can be avoided, with a trade-off: In the current state of this PR, this typed array is simply stored in the respective primitive. When it is needed again, it does not have to be fetched from the GPU. One could consider this, and maybe make it depend on whether there is anything in the scene that involves the "clamping" that causes thepickModel
function to be called so many times.The fact that each vertex is still fetched 3 times (even in the current state of this PR) offers room for further optimization: Any dequantization is applied to the vertices 3 times! One could consider going one step further, and storing the dequantized positions (as a typed array). This will require some further review/attention about where exactly to store this. It cannot be the standard
positionAttribute.typedArray
, because other places of the code might assume that this is not dequantized...Notes
This is currently a draft PR. It is implemented as some sort of "feature toggle", pragmatically splitting the
pickModel
into apickModelOld
andpickModelNew
.One open issue that prevented #11830 from being merged is that it did not handle vertical exaggeration. Now... vertical exaggeration is "wired" into the picking in a way that makes it nearly impossible to pull it out and handle it generically. The current state here handles this with a pragmatic
if (verticalExaggeration !== 1.0) useOldApproach();
. Yeah, it's very pragmatic, but might be OK.The current state also includes some preliminary performance tests: It is measuring the time for
pickModelOld
andpickModelNew
, and prints the average time for the last 1000 calls. Running this with the sandcastle from #12518 shows logs like the following:showing that the new approach is roughly 4 times faster (even without the possible optimizations for quantization).
Issue number and link
In the most narrow sense, this addresses #11814
In a broader sense, it also addresses
because (as noted in a recent comment, these are actually caused by the poor performance of
pickModel.js
).They are generally about performance, and therefore, open-ended.
Testing plan
TODO A sensible performance test...
The specs still seem to be passing. So either the results are right, or we don't have proper coverage 🙂
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change