Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Unity plugin crash and add debug assertions #197

Closed
wants to merge 2 commits into from

Conversation

@toolness
Copy link
Contributor

toolness commented Jun 15, 2019

Okay, I did some investigating into #178 and discovered a few things.

Right now, the Unity plugin instantiates a Renderer only once and reuses it every frame (instantiating one every frame seemed excessive, but I can alter this behavior if you want). However, as of acf666b, the quad_vertex_indices_buffer is only bound to GL_ELEMENT_ARRAY_BUFFER when the renderer is instantiated--not every frame.

This seems fine if we assume that other GL code hasn't changed the binding since the renderer was instantiated. I think this is what's going on with the Unity plugin: because the plugin has to share the same GL context with Unity's own renderer, it's likely that Unity is changing the GL_ELEMENT_ARRAY_BUFFER binding when it's doing its own drawing.

Furthermore, the stack trace mentioned at #178 (comment) is happening because the indices parameter of glDrawElementsInstanced() is defined in a confusing way: if GL_ELEMENT_ARRAY_BUFFER is bound, then it's interpreted as an offset index into an array (which is how we're using it), but if it's not bound anymore--as is the case when running in Unity--it's interpreted as a pointer value, in which case the program will segfault.

I'm not really sure what the best solution is here. I've added a debug assertion called assert_element_array_buffer_is_bound() to make us panic with a helpful error instead of causing a null pointer dereference, which will hopefully help us catch errors like this in the future, but I'm not sure what the best strategy to ensure the proper buffer binding is. If we only plan to ever bind quad_vertex_positions_buffer to GL_ELEMENT_ARRAY_BUFFER then we could just bind it once during Renderer::begin_scene(), but otherwise we may need to bind it immediately before we call glDrawElementsInstanced() or anything else that needs it.

For now I've just done the minimal possible required to make the minimal canvas example work in the Unity plugin, but it's possible the ideal solution is something different. I'm open to suggestions, this PR is really just my way of starting the conversation.

@pcwalton
Copy link
Collaborator

pcwalton commented Jun 17, 2019

Cool, thanks for digging in. My Metal changes in my metal branch will hopefully fix this, since they rework the API around VBO/IBO binding, so let's try again once those have landed.

@toolness
Copy link
Contributor Author

toolness commented Jun 17, 2019

Oh cool, sounds good!

@pcwalton
Copy link
Collaborator

pcwalton commented Jun 20, 2019

@toolness I just pushed the Metal changes. Want to try again?

@toolness
Copy link
Contributor Author

toolness commented Jun 22, 2019

Yes, this works perfectly and logs no weird GL warnings either!! Thanks!

@toolness toolness closed this Jun 22, 2019
@toolness toolness deleted the toolness:better-buffer-binding branch Jun 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.