-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Allow 64k vertices Meshes #5925
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
Conversation
Its compatible ios, android ? If this limitation is break, Its a really good news |
@fabiitch i think so, i can't see any reason why it wouldn't work but i didn't test it on android yet and i can't test on iOS. |
I think this makes sense to merge unless there are objections? Could someone test on iOS as well? |
I'm happy to test on iOS, is running the new BigMeshTest enough as a test? |
@obigu yes, i think it's enough since it tests both model cache and non cache. |
I've run BigMeshTest on an iPad Mini 5 and the Simulator (both iOS 13.4) and I get a totally black screen with the following logs https://pastebin.com/tCPk47b3. Just to confirm is not a problem of the tests on iOS instead of your changes, is there an equivalent test I can run that should work without your PR? |
Test also runs without issues on iOS (iPad Mini 5 iOS 13.4.1). |
Great! If this is ready, can you also update the VERSION file with the change and let's get this in! :) |
@noblemaster done! feel free to squash and reword if necessary. I checked, there are no breaking changes except that now indices can be negative short and need to be properly converted to int in some cases. |
Conflicts: CHANGES
oops, sorry, i didn't correctly pulled before modifying this file xD |
Great :-D |
This is a work in progress PR that allow 64k vertices mesh instead of 32k.
There might be some other impacts like with Bullet, i have to test a bit more. Note that i tested it against desktop and GWT.
I didn't wanted to spread this change to 2D domain (sprite batch and such) for now.
I wanted to have a feedback about this change before going further.
I now that fbx-conv doesn't allow 64k vertices yet but other loaders does (eg. OBJ loader). I needed this change for my GLTF library so i monkey patched Mesh class and it works well so far, so i wanted to share this improvement.
Note that i also fixed an issue in ModelCache using indices count instead of vertices count. I'm not fully satisfied with the fix: we don't know how many vertices are needed without counting indices in mesh part to add. I could isolate this fix in another PR if you think it's prefereable.
I quickly discussed about it with @xoppa recently, i wasn't sure if there was other reasons than java unsigned short for this limitation.
Tell me what do you think about it.