-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Added Scene.pickAsync for non GPU blocking picking #12983
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for the pull request, @alarkbentley! Welcome to the Cesium community! In order for us to review your PR, please complete the following steps:
Review Pull Request Guidelines to make sure your PR gets accepted quickly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @alarkbentley,
Thanks for the contribution! Crazy this goes all the way back to issue #630.
I left a number of comments, mostly style related, a few more important, but nothing that should drastically change the overall shape of the PR.
| let pixels; | ||
| if (pbo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style comment - I'm a stickler for simplifying conditional logic when possible.
This function has basically become two different functions depending on whether pbo is passed in. Let's formalize that:
const readPixelsFunc = pbo ? readPixelsAsync() : readPixelsSync();
readPixelsFunc();Then we have just a single branching point instead of multiple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two funcs can take pixelFormat, pixelDatatype etc. as args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I agree that this would be simpler, could you perhaps show in a commit?
If we go this route I would rather create two separate prototype functions. But they have a lot in common.
| if (debounce && picking && now - debounceTime < DEBOUNCE_TIMEOUT_MS) { | ||
| return true; | ||
| } | ||
| if (debounce) { | ||
| picking = true; | ||
| debounceTime = now; | ||
| } | ||
| if (async) { | ||
| result[0] = await scene.pickAsync(position); | ||
| } else { | ||
| result[0] = scene.pick(position); | ||
| } | ||
| if (debounce) { | ||
| picking = false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps debouncing is something that should be handled internally by the implementation? (As an option, perhaps?)
Or maybe the internal implementation should use a set of results each frame, rather than an array, so that debouncing isn't even necessary (duplicates across frames will hash to the same entry)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the bloat this introduces to the sandcastle. I'm honestly not even sure if such a change is really necessary to show off in a sandcastle. I'm not sure I was even able to tell the difference between (a)sync in the video of this PR's description.
Simplify promise using await Co-authored-by: Matt Schwartz <mzschwartz5@gmail.com>
spelling Co-authored-by: Matt Schwartz <mzschwartz5@gmail.com>
|
I haven't reviewed your unit tests yet, but given some of our conversations, I'm guessing we probably need more testing around:
|
Description
Scene.pick is internally using gl.readPixels that performs a synchronous GPU readback which is blocking the main render thread and very costly especially on low end GPUs. With WebGL2 a non GPU blocking method is available that uses PBO and Sync operations to asynchronously readback the pixels data without blocking.
Reference: https://registry.khronos.org/webgl/specs/latest/2.0/
Section: 3.7.10 Reading back pixels
This PR implements support for this and exposes it under a new API method Scene.pickAsync -> Promise
Asynchronous picking gives precedence to the rendering over returning the pick information. This introduces a slight delay compared to using the blocking pick method. Depending on what you prefer there might be value in having both options available in API.
Example
Demo
Browser: Microsoft Edge Version 141.0.3537.71 (Official build) (64-bit)
GPU: ANGLE (Intel, Intel(R) Arc(TM) Pro Graphics (0x00007D55) Direct3D11 vs_5_0 ps_5_0, D3D11)
edge_sync_async.mp4
Profiling results
As seen in the synchronous picking (left), readpixels can stall for longer time than it takes to render the entire frame. In this example 46% of the total execution time is taken by readpixels.
In the asynchronous example (right) the bottleneck is the actual frame rendering which makes more sense.
Issue number and link
#630
Testing plan
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change