Skip to content

Revive model picking refactoring #12658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Jun 7, 2025

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 pseudocode

for (all instance transforms t) {
  for (all triangle indices i0, i1, i2) {
    v0 = fetchVertex(i0);
    v1 = fetchVertex(i1);
    v2 = fetchVertex(i2);
    transformVertex(t, v0);
    transformVertex(t, v1);
    transformVertex(t, v2);
    intersect(ray, v0, v1, v2);
  }
}

This looks innocent, but isn't:

  • Each vertex appears in 3 triangles, and is therefore fetched 3 times
  • Each vertex is transformed once for each instance transform

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 the pickModel 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 a pickModelOld and pickModelNew.

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 and pickModelNew, and prints the average time for the last 1000 calls. Running this with the sandcastle from #12518 shows logs like the following:

Average duration of 1000 calls
  old: 0.419399999961257 ms
  new: 0.1159000000320375 ms
Average duration of 1000 calls
  old: 0.41479999994672834 ms
  new: 0.11820000004768372 ms
Average duration of 1000 calls
  old: 0.4205999999828637 ms
  new: 0.12280000001378358 ms

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

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

github-actions bot commented Jun 7, 2025

Thank you for the pull request, @javagl!

✅ We can confirm we have a CLA on file for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant