Skip to content
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

Support for Picker on WebGPU #6393

Merged
merged 4 commits into from
May 21, 2024
Merged

Support for Picker on WebGPU #6393

merged 4 commits into from
May 21, 2024

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented May 17, 2024

  • New public API for both WebGL and WebGPU:
// async version of the existing API returning a Promise resolving to an array of MeshInstances
Picker.getSelectionAsync(x, y, width, height)
  • existing API (WebGL only, blocking):
Picker.getSelection(x, y, width, height)

example of use:

picker.getSelectionAsync(10, 20, 5, 5).then((meshInstances) => {
    console.log(meshInstances);
});
  • updated the picker and gizmos example to use the async version on both platforms.

Copy link

vercel bot commented May 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
engine ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2024 10:58am

* with the pixel data of the texture.
* @ignore
*/
read(x, y, width, height, options = {}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is read meant to supersede downloadAsync? Seems there is some duplication here. Couldn't there just be a single function that does it all?

Also, I prefer the name downloadAsync (to match upload) more than read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, downloadAsync is "deprecated" and will be removed. I left it there as there likely is some user of this.

read matches StorageBuffer read and write, download seems more internet term which seems confusing to use on the Texture.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed downloadAsync - only the model viewer seems to be using it, and we can easily swap it to use read when we update it to the new engine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you'll rename upload to write as well? You think the functions read/write describes what is happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write function would take user data, set it on the object and upload internally.
upload just uploads already provided data
read rives user a copy of data for reading

I think read is a pretty descriptive and simple name for what the user wants to do - read the content of the texture. Note that for now this is a non-public API, and we can revise this in a follow up PRs.

@@ -76,50 +91,110 @@ class Picker {
*/
getSelection(x, y, width = 1, height = 1) {
const device = this.device;
if (device.isWebGPU) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems real messy. I would prefer one way of getting texture data on both (even if one implementation must emulate the other).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is backwards compatibility, which a lot of users depends on, and so I left it there. Deprecating this would create a breaking change for no reason for now, people can move over to the newer API without us forcing it at this stage, as that could be a larger impact change. Including the Editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one way to get the data on both, called Texture.read - this is async and works on both WebGL2 and WebGPU.
Public API Picker.getSelectionAsync uses it, and works on both platforms.
Existing Picker.getSelection is left there an a synchronous WebGL2 only method, to avoid a breaking change, and prints an error on WebGPU platform, forcing people to upgrade at that point.

@mvaligursky mvaligursky merged commit 8f83915 into main May 21, 2024
9 checks passed
@mvaligursky mvaligursky deleted the mv-picker-webgpu branch May 21, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants