Skip to content

Conversation

@cabanier
Copy link
Member

@cabanier cabanier commented Feb 19, 2021

Closes #249


Preview | Diff

@cabanier cabanier requested review from Manishearth and toji February 19, 2021 17:40
@toji
Copy link
Member

toji commented Feb 19, 2021

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.

@cabanier
Copy link
Member Author

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.

Thanks!

@cabanier cabanier merged commit dd11428 into immersive-web:main Feb 19, 2021
@cabanier cabanier deleted the ignore-depth branch February 19, 2021 19:34
@toji
Copy link
Member

toji commented Feb 19, 2021

Perfect, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants