Support data textures and off-screen rendering

[xian]

• Add support for texImage2D uniforms with non-image data.
• Add Framebuffer and Renderbuffer support.
• Buffer is now abstract and implemented by FloatBuffer and UByteBuffer, which both have reasonable implementations for JS and JVM.
• Buffer.put(array, length) now appends at the buffer's current position instead of overwriting starting at 0.

[gergelydaniel]

Thank you for your contributions, this one seems to be a big one, I'll read it through and I'll be back with some comments!

[xian]

Thanks for having a look at my PRs! Btw, I'm using kgl in an LED control/light show authoring system which incorporates glsl shaders, and needs to run both on jvm (in production) and in js (dev/simulator): https://github.com/baaahs/sparklemotion

[xian]

One potentially breaking change I made is that Buffer.put() with an array now appends to the buffer rather than overwriting from the beginning, but that seems likely to be the intended behavior?

[gergelydaniel]

Nice, sounds exciting, do you use either LWJGL or JOGL on JVM?

Yes, the overwriting thing was definitely a bug.

[gergelydaniel]

I like the idea of creating a read-write multiplatform Buffer.

My first question would be whether it is a good idea to use UByte and UByteArray, since inline classes are not perfect, they don't really work at all in JS, and even in JVM it requires some extra attention (for example to avoid auto-boxing, and there are some bugs when you use inline classes and generics)

I did some thinking and realized handling bytes on Kotlin Multiplatform won't be a trivial task no matter what.

Did you consider other alternatives, like simply just using Byte, what are your thoughts?

[xian]

I'm using LWJGL right now but thinking of switching to JOGL because of the -XstartOnMainThread/gradle [test issue])https://github.com/gradle/gradle/issues/864). Honestly I'm pretty new to OpenGL so still learning stuff. :-)

[xian]

I'd be happy to switch to Byte since UByte seems not quite baked yet. I'm using it for reading back pixel data, so unsigned is really what I want semantically, but maybe that should wait until they make it officially supported. I'll send another commit with that change later today. :-)

[xian]

@gergelydaniel — updated!

[gergelydaniel]

I checked it out, the only significant thing I'd still change are the implementations for Buffers' put method that are accepting arrays. Instead of putting the elements one by one in a loop, we should use a single platform-specific API call to update all the values at once. For large amount of data it can offer significant performance improvement.

I think the previous JS implementation was fine:

actual fun put(floatArray: dynamic, length: Int) {
     buffer.set(floatArray.subarray(0, length), pos)
     pos += lenght
}

And even though the previous JVM implementation was incorrect (I think because using asFoatBuffer() resets the position), I still think the usage of this method is the right direction: put(float[] src, int offset, int length)

[xian]

Fixed Buffer.put() and a couple bugs in the JVM impl.

[xian]

One issue: before calling texImage2D(..., buffer) on JVM, it's necessary to reset buffer's position to 0. Perhaps this should be dome implicitly?

[gergelydaniel]

Sorry for the delay, needed some time to think it through :)

Buffer.put implementations are now fine for JS, but can you fix them on JVM as well? 😅 (to use single Byte/FloatBuffer.put(array src, int offset, int length) call instead of a loop)

About the position resetting issue: This library supposed to be as lightweight and as thin of a layer as possible, also as close to the libraries it's wrapping as possible. Therefore I think that it should be done explicitly by the caller. This way we also wouldn't take away the flexibility of being able to call texImage2D with only part of a Buffer's data. For that there needs to be a solution so that JS can show the same behavior as JVM: one that considers the Buffer's position. I think it should be done by exposing the position of the buffer, and create a subarray starting from that position. I created a draft for it: 62a0955 What do you think?

[xian]

Oops, fixed the JVM impl of put(). 62a0955 looks good to me, merged it on this PR. :-)

[xian]

TBH I’m of two minds on the position question. It seems almost certain to me that 99% of people will want to rewind the buffer for every use, in support of a theoretically useful feature (arbitrary positioning inside a native buffer in excess of what OpenGL already offers), but will probably only realize that they need to call position(0) only after they get a confusing error message. On the other hand, I don’t particularly like resetting the buffer position as a side-effect of calling a GL function.

What if we consider position relevant to buffer API reads and writes only, but not to GL bindings which are always to the buffer in full, and document it well in kdoc?

Since OpenGL will be reading freely from the buffer at arbitrary positions in arbitrary order some arbitrary number of times in the future, it seems like a reasonable departure from expected behavior for a class called Buffer, and also the presumable behavior of an OpenGL API.

[xian]

@gergelydaniel — I added createVertexArray() etc. (in 53bf573) which I needed to get the LWJGL version working on Mac (which doesn't support compatibility profiles, required to skip the VAO setup as far as I can tell); I haven't managed to get my app working with JOGL yet. Vertex arrays require WebGL2, so I hacked a bit of that in for the JS version. Let me know if any of that needs further discussion; I could pull it out as a separate PR.

Also, I'd be happy to open another PR with the buffer ideas I mentioned in https://github.com/gergelydaniel/kgl/pull/5#issuecomment-534429682 so we can get this PR landed.

[gergelydaniel]

I did some more thinking about the Buffer issue. The problem is probably that kgl's Buffer from a common point of view is a mixture of a JS TypedArray and a JVM Buffer, which can be confusing. If you consider it as a typed array it's natural that you don't have to "rewind" it, just pass it in and it will be read from the beginning, but if you consider it like a JVM Buffer, it's natural that you have to reset the position. WebGL and the native OpenGL itself probably does it best, since you don't have to do anything if you just want to read the data from the beginning, while you are still able to pass it in with an offset.

Since I don't like the idea of taking away that flexibility, what about a third option: Every Kgl function that takes a Buffer as a parameter, gets a offset: Int parameter as well, which is 0 by default?

[gergelydaniel]

Btw once we settle this issue I'll just do some testing myself, and then the PR should be ready to merge :)

[xian]

Awesome solution. I've added a couple commits to implement that.

Btw, I'd be happy to write some unit tests for this stuff if you'd like (but maybe in a separate PR)?

[xian]

The uint vs int question maybe deserves revisiting at some point, but not urgently I think.

[xian]

@gergelydaniel — ping :-)

[gergelydaniel]

Sorry for delay, unfortunately I couldn't really schedule any time for this project this week, but I'll try harder next time :)

I found two bugs during testing, please see the comments here, otherwise it seems to be working fine for me.

Also there's one more thing: I like the withIoBuffer method, but we should make it inline, because that way it avoids creating an actual callback object. The buffer field should be made public to make it work.

Otherwise it should be fine, and finally this PR can be merged :D

[gergelydaniel]

Some unit tests would be great, I agree that it should be done separate from this PR

[xian]

Hah, oops. Fixed!

I can't say that I agree with making the encapsulated platform-specific buffer externally visible; the whole point of the Buffer classes is to hide it. I'd make them all internal but for some reason the KglAndroid call sites aren't considered internally visible. You're concerned about overhead? readPixels(), bufferData(), and texImage2D() all strike me as fairly infrequently used high-cost calls themselves... is it worth it?

[xian]

I guess buffer and the with_() methods will never be visible to common code so it's not so terrible to make it visible. I'll make the change if you're in favor of if.

[gergelydaniel]

I agree that it's not ideal to have the buffer field exposed, but since it's not visible from common code, only from JVM-specific code, it's less significant, we could also write a warning in kdoc, not to use the field directly, or to use it with caution.

So all in all I think it's worth the tradeoff.