Skip to content

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

Merged
merged 7 commits into from
Apr 21, 2020
Merged

Allow 64k vertices Meshes #5925

merged 7 commits into from
Apr 21, 2020

Conversation

mgsx-dev
Copy link
Contributor

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.

@fabiitch
Copy link
Contributor

Its compatible ios, android ? If this limitation is break, Its a really good news

@mgsx-dev
Copy link
Contributor Author

@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.

@ademola-lou
Copy link

Screen Shot 2020-03-28 at 6 31 15 AM

works perfectly fine on android emulator and old android devices

@noblemaster
Copy link
Member

I think this makes sense to merge unless there are objections? Could someone test on iOS as well?

@obigu
Copy link
Contributor

obigu commented Apr 20, 2020

I'm happy to test on iOS, is running the new BigMeshTest enough as a test?

@mgsx-dev
Copy link
Contributor Author

@obigu yes, i think it's enough since it tests both model cache and non cache.

@obigu
Copy link
Contributor

obigu commented Apr 21, 2020

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?

@mgsx-dev
Copy link
Contributor Author

i fixed screen clearing in my test (sorry about that), result should looks like this :
Screenshot 2020-04-21 15:41:46

@obigu
Copy link
Contributor

obigu commented Apr 21, 2020

Test also runs without issues on iOS (iPad Mini 5 iOS 13.4.1).

@noblemaster
Copy link
Member

Great! If this is ready, can you also update the VERSION file with the change and let's get this in! :)

@mgsx-dev
Copy link
Contributor Author

mgsx-dev commented Apr 21, 2020

@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.

@mgsx-dev
Copy link
Contributor Author

oops, sorry, i didn't correctly pulled before modifying this file xD

@noblemaster noblemaster merged commit 6986702 into libgdx:master Apr 21, 2020
@noblemaster
Copy link
Member

Great :-D

MrStahlfelge pushed a commit that referenced this pull request Oct 2, 2020
* fix error message

* add white space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants