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

Allow non-null FLOAT_32_UNSIGNED_INT_24_8_REV uploads. #1678

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kdashg
Copy link
Contributor

@kdashg kdashg commented May 12, 2016

It sounds like the only reason we disabled this is because there's no good match for the type of the ArrayBufferView to be uploaded. We do seem to allow other depth/stencil upload types.

We should just let people upload it if they manage to marshal it, perhaps just allowing ArrayBuffer here.

@@ -1376,6 +1375,7 @@ <h2 class="no-toc">Table of contents</h2>
<tr><td>Uint32Array</td><td>UNSIGNED_INT_24_8</td></tr>
<tr><td>Uint16Array</td><td>HALF_FLOAT</td></tr>
<tr><td>Float32Array</td><td>FLOAT</td></tr>
<tr><td>ArrayBuffer</td><td>FLOAT_32_UNSIGNED_INT_24_8_REV</td></tr>
Copy link
Member

Choose a reason for hiding this comment

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

ArrayBuffer is not compatible with the ArrayBufferView argument in the IDL. This change does not look good as is.

I am dubious that the added value of supporting uploads for this texture type is worth the perturbation to the IDL. Another overload would have to be added in order to support just this texture type -- assuming we don't want to use one of the other existing ArrayBufferView types for it (which would probably be confusing). The best fit would be either Float32Array or Uint32Array.

@lexaknyazev
Copy link
Member

From #2667 (comment):

In the last WebGL working group meeting it was resolved to change the spec to allow ArrayBuffer for these entry points that currently only allow ArrayBufferView.

Is it still expected? If so, this PR could be extended to cover that issue.

@kenrussell
Copy link
Member

We do still aim to make that change, but right now all browser vendors are making a push to pass the top-of-tree conformance suite before taking the next snapshot. It's not a good time right now to make significant API changes - and this would be one.

@lexaknyazev
Copy link
Member

@kenrussell @jdashg
Do you still expect to implement this change, along with #2667?

@kdashg
Copy link
Contributor Author

kdashg commented Jun 17, 2021

Yes, some day. :) Thanks for the bump!

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.

3 participants