You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
So to be clear: this change makes it so that if you request a depth texture (which is still the default) but don't have the depth extension enabled for a WebGL 1 context then you'll get back a layer with only the color texture.
That works, though it feels a bit weird. Tried thinking through the worst case scenarios:
Worst case is that you didn't enable the extension and question why you didn't get a depth buffer when the default says you should have. In that case the browser should definitely be outputting a warning to explain what happened. We can't put that in the spec text, but maybe a non-normative note suggesting it would be good. And in any case, that's not an error that you're likely to ship with.
More problematic is that the object returned by enabling the extension only has an enum on it, so it's easy to ignore the result and simply assume that the extension was enabled. So you may request the extension, assume it works, and observe that the code runs on your machine only to find that it fails on a different device. HOWEVER! It's extremely unlikely that any device that can even pretend to support VR won't have the ability to do depth textures, so this is more of a theoretical issue than a real world concern.
Finally, there's a the issue that someone may not enable the extension initially and depend on this behavior to prevent allocation of the depth buffer (which they're not using) only to later enable the extension and suddenly be allocating an unnecessary texture. This would be further compounded by the fact that we assume that if a depth texture is allocated we can use it for compositing. I think this is probably a rare situation, but it's worth bringing up.
That said, I don't think the above concerns are worth blocking this change for. I simply wanted to list them out to acknowledge that we thought about them and/or give you an opportunity to address them if you happen to have an idea about how to address it.
Worst case is that you didn't enable the extension and question why you didn't get a depth buffer when the default says you should have. In that case the browser should definitely be outputting a warning to explain what happened. We can't put that in the spec text, but maybe a non-normative note suggesting it would be good. And in any case, that's not an error that you're likely to ship with.
Good point!
I'll add a note to the spec and when I implement it, I will make sure to log a warning to the console.
That said, I don't think the above concerns are worth blocking this change for. I simply wanted to list them out to acknowledge that we thought about them and/or give you an opportunity to address them if you happen to have an idea about how to address it.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #249
Preview | Diff