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

Fixes crash with rigged meshes on some OpenGLES2 devices #29751

Merged
merged 1 commit into from Jun 18, 2019

Conversation

@lawnjelly
Copy link
Contributor

lawnjelly commented Jun 13, 2019

Fixes #28298

Non-tools OpenGLES2 devices that use the USE_SKELETON_SOFTWARE path (i.e. do not support float texture) depend on surface->data being set containing the bone IDs and weights (rasterizer_scene_gles2.cpp, line 1456, RasterizerSceneGLES2::_setup_geometry):

PoolVector<uint8_t>::Read vertex_array_read = s->data.read();
const uint8_t *vertex_data = vertex_array_read.ptr();

However currently if TOOLS_ENABLED is not defined, surface->data is not stored in main memory in rasterizer_storage_gles2.cpp. This causes a crash in rasterizer_scene_gles2.cpp when a rigged object comes into view.

This fix addresses the specific case of skinned objects when USE_SKELETON_SOFTWARE is active, and stores a copy of the bone data, as is done when TOOLS_ENABLED is defined. This fixes the crash by allowing the same mechanism as on desktop, without adding the memory overhead of storing all vertex data where not required.

@mbrlabs

This comment has been minimized.

Copy link
Contributor

mbrlabs commented Jun 13, 2019

Nice, i just tried it! I had the same issue on my Galaxy S8 and that fixed it.

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jun 17, 2019

Great work debugging this issue and finding a way to solve it!

Some comments from @reduz and @clayjohn on IRC:

[17:47:28] <Akien> reduz: could you check #29751 ? It's pretty simple and fixes a critical issue for GLES2 on mobile, would be great to have in 3.1.2
[17:47:29] <IssueBot> #29751: Fixes crash with rigged meshes on some OpenGLES2 devices | https://git.io/fj2ZD
[18:08:26] <reduz> Akien: it seems simple, so it fixes it it's good, but it shouldn't have comments inside saying it fixes a bug, just explaining what it does
[18:13:05] <clayjohn> Akien: I'm looking at it as well. And I'm not sure that it is the proper fix, shouldnt I think all that needs to be done is to move ``surface->data`` and ``surface->index_data`` outside of the ``#ifdef TOOLS_ENABLED``
[18:13:39] <clayjohn> His solution only writes to ``surface->data`` when skeleton is active, but it looks like it is used more than that.
[19:24:23] <Akien> clayjohn: Yeah that's probably the cleanest change if this data is necessary.
[19:28:09] <clayjohn> Akien (@freenode_Akien:matrix.org): reduz is the one who put it inside the ifdef. Also the functions that access that data have ERR_FAILs attached saying that they cannot be used in GLES2.
@lawnjelly

This comment has been minimized.

Copy link
Contributor Author

lawnjelly commented Jun 17, 2019

Addressing the comments in the post above:

Akien: I'm looking at it as well. And I'm not sure that it is the proper fix, shouldnt I think all that needs to be done is to move surface->data and surface->index_data outside of the #ifdef TOOLS_ENABLED

My original fix did exactly that and moved it so that it always wrote to surface->data even if not in tools.

However it occurred to me (as clayjohn points out) that this may not need to be stored except in the cases where needed for skeleton animation, or where it solves other problem, because (if I understand it correctly) it is maintaining a main memory copy of the vertex data that may be unneeded in most cases, and is potentially a waste of memory on low memory devices (consider if you have a lot of high poly geometry). I was guessing that this was part of the original reason for it being #ifdef for only tools.

I think there's arguments for either approach so I'm happy to go with whichever you guys decide, please let me know and I can edit it, and whether you wish for the index data to be stored too (this isn't needed for the skinning).

Agree on my comments being overly verbose, being a first fix I wasn't sure on the standards for how much to put into the source code itself. I guess the information is available on the github history too. I'll try and reduce the comments to something sensible. 😄

Non-tools OpenGLES2 devices that use the USE_SKELETON_SOFTWARE path (i.e. do not support float texture) depend on surface->data being set containing the bone IDs and weights (rasterizer_scene_gles2.cpp, line 1456, RasterizerSceneGLES2::_setup_geometry). However currently if TOOLS_ENABLED is not defined, surface->data is not stored in main memory in rasterizer_storage_gles2.cpp. This causes a crash in rasterizer_scene_gles2.cpp when a rigged object comes into view.

This fix addresses the specific case of skinned objects when USE_SKELETON_SOFTWARE is active, and stores a copy of the bone data, as is done when TOOLS_ENABLED is defined. This fixes the crash by allowing the same mechanism as on desktop, without adding the memory overhead of storing all vertex data where not required.

Fixes #28298
@lawnjelly lawnjelly force-pushed the lawnjelly:skin-fix branch from 0fceaa0 to e36e9fd Jun 18, 2019
@lawnjelly

This comment has been minimized.

Copy link
Contributor Author

lawnjelly commented Jun 18, 2019

Ok I've modified the comments to be more sensible and have swapped the if checks as well (just me being pedantic).

My thoughts are it's either fine to merge as it is, or it can be changed to always copy. However always copying would probably be meaningless unless other changes were made, as clayjohn points out access is wrapped in ERR_FAILs on GLES2.

I am tending towards thinking that changing it to always copy (if this is indeed necessary) should be a separate issue because of the wider implications, and a separate discussion and might be better with a separate PR.

@akien-mga akien-mga requested a review from clayjohn Jun 18, 2019
@akien-mga akien-mga merged commit 0cdbf73 into godotengine:master Jun 18, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jun 18, 2019

Thanks!

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 17, 2019

Cherry-picked for 3.1.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.