-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
Optimizations to mikktspace.c #93615
base: master
Are you sure you want to change the base?
Conversation
Created in response to: godotengine#93587 Large glb file (778 MB) would hang, crash or load after extremely long time. This first set of optimizations focuses on sending SVec3 objects by reference instead of by value. Local tests in debug mode caused one iteration of GenerateSharedVerticesIndexList to go from 552ms to 470ms, on average. Unsure of the performance gains on release mode.
Created in response to: godotengine#93587 Large glb file (778 MB) would hang, crash or load after extremely long time. This first set of optimizations focuses on sending SVec3 objects by reference instead of by value. Local tests in debug mode caused one iteration of GenerateSharedVerticesIndexList to go from 552ms to 470ms, on average. Unsure of the performance gains on release mode.
freakin whitespace
First time using Github like this. Hopefully this isn't too bad. |
These changes must be added as a patch file in |
If it can be done it would be worth considering reporting this issue to the third-party, as well. |
The patch would help a lot with that as well |
For the record, upstream is now https://github.com/mmikk/MikkTSpace and no longer just a page on the (old, and now 404'ing) Blender wiki. But before proposing this upstream, this should definitely be benchmarked in a production build, debug build times aren't super useful aside from seeing the impact on debugging usability for engine devs. |
Is there a standard benchmarking system of any kind? Any info would be helpful. |
Can you try to salvage #83648 |
This is reference code for the implementation used by Blender, which is in C. This is also not a library that changed at all in the past 10 years, so it's not a problem if we hard fork it to optimize it for Godot's needs. |
mmikk/MikkTSpace#5 a 5 month old Issue where someone is asking for 2 '=' to be added to correct some sorting. (I will say I haven't vetted the request but it hasn't been commented on either.) Confidence is low. But, I'm not here to talk trash about a legacy/third-party lib. It was amazing until we want to load larger files. What's the real solution? |
I think it's fine modifying this lib downstream for our needs, it's a single file and not one that gets updated frequently. If porting to C++ lets us use features that improve performance in Godot's context, then I don't see a reason not to do it. |
There is still lots of room for improvement but with my local tests I am down to 322ms from 552ms before my other proposed changes. The new major bottlenecks are QuickSortEdges and mikktGetPosition. Issues: I am temporarily unable to build a release build, locally. I will look into this to get real world numbers. I don't know how copyright works in this instance. I would happily keep the original copyright and hand these changes over. I left a large comment in surface_tool.h about how to handle this side-change to the use of these new files without removing the old mikktspace.
I have so much to learn about git. |
The error is ambiguous and oddly seemed to point the order in which I included two files. I need more verbosity or I just missed the description.
I've had good experiences writing doctest unit tests for these things complicated things, it's not a requirement for merging, but wanted to mention. |
Note that you can get Godot to print the time taken to import each resource by starting it with Make sure to remove the |
|
I was in search of every possible way to improve load times (of glb files in my case) and created this monstrosity of a set of changes. Do not allow this change to go through. I just wanted to preserve this set of changes for personal reasons.
Hashing out some potential results with the owner of a potentially huge fix. |
Created in response to: #93587
Large glb file (778 MB) would hang, crash or load after extremely long time. This first set of optimizations focuses on sending SVec3 objects by reference instead of by value. Local tests in debug mode caused one iteration of GenerateSharedVerticesIndexList to go from 552ms to 470ms, on average. Unsure of the performance gains on release mode.