greggman / twgl.js

A Tiny WebGL helper Library
http://twgljs.org
MIT License
2.67k stars 260 forks source link

AttachmentOptions missed logic #168

Open kdziamura opened 4 years ago

kdziamura commented 4 years ago

During the investigation, I found that parameter AttachmentOptions['attach'] is never used, but fully documented and has typescript declaration: https://github.com/greggman/twgl.js/blob/9ed693912454c5b041776683f3b3633df7e6ca7e/src/framebuffers.js#L88 https://github.com/greggman/twgl.js/blob/9ed693912454c5b041776683f3b3633df7e6ca7e/npm/base/index.d.ts#L22

An actual attachment point is being selected here: https://github.com/greggman/twgl.js/blob/9ed693912454c5b041776683f3b3633df7e6ca7e/src/framebuffers.js#L200

I'll be glad to create a pull request, but I confused with the idea of this parameter. Is it redundant legacy code and should be removed, or is it an option, that should be used as an attachment point if defined?

munrocket commented 4 years ago

Yeah, looks like an outdated comment that become a property in typescript.

Here usage of attachments https://github.com/greggman/twgl.js/blob/9ed693912454c5b041776683f3b3633df7e6ca7e/examples/mulitple-render-targets-3d-texture.html#L169

kdziamura commented 4 years ago

So the order of attachments is important. Maybe it's better to have the option of using a specific attachment point? Or, at least, add attachment point into output object? It would be great to have the availability of reusing it in some cases. To use with gl.drawBuffers for example. Here is the manual definition of them, and it could be unclear at first glance: https://github.com/greggman/twgl.js/blob/9ed693912454c5b041776683f3b3633df7e6ca7e/examples/mulitple-render-targets-3d-texture.html#L175 I mean that current logic isn’t transparent. The other option is to add some parameter/s, which will run gl.drawBuffers under the hood, but it’s unclear and not flexible solution as well

greggman commented 4 years ago

Good catch. I'm not sure off top of my head what the best solution is. twgl is not trying to cover all of WebGL. It's just trying to make some things less verbose.

I don't think generating an input for drawBuffers is real all that useful. It's a really tiny API. Just an array with 4-8 constants. Does that really need an helper?

The simplest thing would be to remove it. Clearly no one is using it if it never worked. .

Otherwise the intent was

let attachmentPoint = attachmentOptions.attach || getAttachmentPointForFormat(format);