Buffer.GetData() GLES fix

k4G17eYWI commented 5 months ago

There is no glMapBuffer() on GLES but it has the glMapBufferRange() as a more modern way to do the same. So I replaced the function and it is working. I've tested it on Windows 11 desktop and on Android phone with GLES 3.2

cpt-max commented 5 months ago

Great, thanks for that. I'll check it out. On a quick glance, most of the changed lines seem to be whitespace changes coming from an auto-formatter. It's best to not commit those, makes it difficult to see the actual changes.

cpt-max commented 5 months ago

glMapBufferRange was introduced with OpenGL 3. At this point OpenGL 2 is still supported by MonoGame. It's OK to use this function, but there must be a fallback to glMapBuffer when it's not available

k4G17eYWI commented 5 months ago

As I can see at the time your PR is merged they will drop this legacy opengl support, won't they?

cpt-max commented 5 months ago

I think the idea is to drop OpenGL completely and replace it with Vulkan. It doesn't make sense for me to drop OpenGL 2 support over this. The fallback is so easy to implement.

k4G17eYWI commented 5 months ago

I've fixed indirect drawing on Android by adding this to PlatformInitialize():

            // Create VAO
            // Indirect drawing does not work without VAO on some platforms (Android for instance)
            // (for some reason this does not work for a scalar, works only for an array)
            var a = new uint[1];
            var dataPtr = GCHandle.Alloc(a, GCHandleType.Pinned);
            var vao = (UIntPtr) dataPtr.AddrOfPinnedObject();
            GL.GenVertexArrays(1, vao);
            GL.BindVertexArray(a[0]);

PR updated. Maybe we should dispose it on application exit? Not sure about this.

cpt-max commented 5 months ago

Thanks for looking into this. So you are creating one global VAO that's constantly bound. I'm a bit worried about inefficiency here. Every time the vertex attribute configuration changes this VAO will be updated. I think generally you want to have different VAO's for different vertex attributes. Using the same VAO with different vertex buffers sounds OK, I'm skeptical about using it with different vertex attributes.

k4G17eYWI commented 5 months ago

I don't think will be a performance degradation here because driver do the same as without this VAO, only store data in different place. But without any measurements it's only my speculation.

If you think we need approach with different VAO's, could you explain it a bit more? Does it match an existing architecture?

k4G17eYWI commented 5 months ago

Btw: what is the proper way to use these changes in my project? Should I build a nuget? When I use build on in my fork repo I get this: MonoGame\MonoGame.Framework\Graphics\BufferResource.cs(48,13): error CS0103: The name 'PlatformConstruct' does not exist in the current context [...\MonoGame\MonoGame.Framework\MonoGame.Framework.Native.csproj]

It seems to be related with my changes but I don't understand exactly how to deal with it.

cpt-max commented 5 months ago

When a VAO is bound, every time you change vertex attributes or the vertex buffer, the VAO will update . The whole point of the VAO is to remember this state, so when you draw an object, you only need to set the corresponding VAO, instead of setting vertex attributes and buffers independently every time.

If the VAO usage was limited to Android and only to indirect draw calls, then a quick fix solution like this might be more acceptable, but as it is, it will probably add some overhead to every draw call. At least that's my expectation.

You don't need the MonoGame.Framework.Native project. Just remove it from the solution. Eventually those missing methods should probably be added. This was introduced with the latest merge of the MG develop branch into the compute branch.

EDIT: and no, you don't need to build a Nuget, you can use the MonoGame.Framework.Android project directly.

k4G17eYWI commented 5 months ago

If you have time please check out this discussion https://hero.handmade.network/forums/code-discussion/t/896-opengl_vbo_without_vao

They said that default zero VAO is still VAO. And it is deprecated:

Client vertex and index arrays - all vertex array attribute and element array index pointers must refer to buffer objects. The default vertex array object (the name zero) is also deprecated. Calling VertexAttribPointer when no buffer object or no vertex array object is bound will generate an INVALID_- OPERATION error, as will calling any array drawing command when no vertex array object is bound.

k4G17eYWI commented 5 months ago

So what's the plan? I will remove unnecessary formatting from this PR but should we keep my solution or switch to another like you suggest. Will you merge it in the near future or it's better to use my own fork for my game?

And, honestly, I don't think monogame maintainers will merge you PR anytime. It is assigned to 5.0 version which suppose to drop OpenGL and use Vulkan. So they (or we) have to completely redo all the code. What do you think about it?

cpt-max commented 5 months ago

If you have time please check out this discussion https://hero.handmade.network/forums/code-discussion/t/896-opengl_vbo_without_vao

There's this statement in there

The easiest way to do things is to just create a VAO when your program starts and then just use VBOs as normal, without messing around with the VAO stuff. Last I heard it is actually faster to do things that way than using VAOs the "correct" way.

That's exactly what you did. If this is true then your solution is fine. I simply don't know if it's true. I'm not aware of any indirect draw problems on the desktop, so I'd prefer to keep things as they are on dektop, and only apply this fix for Android (either use #if ANDROID or #if GLES).

cpt-max commented 5 months ago

So what's the plan? I will remove unnecessary formatting from this PR but should we keep my solution or switch to another like you suggest. Will you merge it in the near future or it's better to use my own fork for my game?

I'll merge it. The remaining TODO's are

remove formatting changes
check if MapBufferRange function exists, if not use MapBuffer function
limit VAO fix to ANDROID or GLES, or come up with more solid proof that it won't have a negative impact on performance (so far it's just a statement from some guy on the internet)
move MapBufferRange, GenVertexArrays, BindVertexArray from LoadEntryPoints() to LoadExtensions(). Potentially unavailable functions are loaded in LoadExtensions

And, honestly, I don't think monogame maintainers will merge you PR anytime.

Neither do I. I don't understand how that's relevant for the discussion here. Is it about whether to implement the fallback or not? The options are:

put in an if statement and stay compatible with current MG
save the if statement and loose compatibility

That's an obvious decision for me. If there was some big gain from dropping compatibility, we can talk.

It is assigned to 5.0 version which suppose to drop OpenGL and use Vulkan. So they (or we) have to completely redo all the code. What do you think about it?

When MG switches to Vulkan there's definitely some work to do to get the compute branch updated. I don't know yet if I have the time to make that happen. That's a decision for another day.

cpt-max commented 5 months ago

I just pushed the missing functions for the native project. It should compile now.

cpt-max commented 5 months ago

@k4G17eYWI are you going to update your PR?

k4G17eYWI commented 5 months ago

I'll do it within a few days.

orosbogdan commented 1 month ago

Is this related to the build failing for the effect in the android sample ?

cpt-max commented 1 month ago

No, this is the issue you are facing here: https://github.com/cpt-max/Docs/issues/3

cpt-max / MonoGame

Buffer.GetData() GLES fix #5