-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
* with the pixel data of the texture. | ||
* @ignore | ||
*/ | ||
read(x, y, width, height, options = {}) { |
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.
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
.
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.
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.
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 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
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.
So you'll rename upload
to write
as well? You think the functions read
/write
describes what is happening?
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.
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) { |
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.
This seems real messy. I would prefer one way of getting texture data on both (even if one implementation must emulate the other).
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.
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.
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.
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.
example of use: