godot
godot copied to clipboard
godotengine
Optimizations to mikktspace.c
Created in response to: https://github.com/godotengine/godot/issues/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.
These changes must be added as a patch file in thirdparty/misc/patches as they are third party sources and can be updated upstream, losing our changes
If it can be done it would be worth considering reporting this issue to the third-party, 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. The last changes to that repo is 4 years ago but I'll attempt a pull request. Can I propose to replace these two files with a cpp implementation?
Is there a standard benchmarking system of any kind? Any info would be helpful. The last changes to that repo is 4 years ago but I'll attempt a pull request. Can I propose to replace these two files with a cpp implementation?
This is reference code for the implementation used by Blender, which is in C. So I don't think porting to C++ would be welcome, though a separate (duplicated) reference implementation in C++ may be ok, you can ask if they're interested.
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.
https://github.com/mmikk/MikkTSpace/issues/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? My findings (which can be flawed) are showing the major bottlenecks being sqrt calculation with Length calls and calls to VScale. VScale has nothing going on computationally. The issue must be copy elision. Creating an SVec3 and returning a copy. C doesn't allow me to make overloaded constructors for structs (as far as I can see. I'm a C++ guy way more than C). I'm looking at other ways to get around this but open to suggestions.
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.
I have so much to learn about git. I have created a new commit that isn't a new pull request... It is an initial C++ implementation of mikktspace There are several options to consider before accepting this change. I will try to get real world timings for these changes but I wanted to get some opinions from you guys before I dive further and spend time on something that may be impossible to actually complete. While writing this I see I have a lot of fails. Looks like linting errors.
I would recommend getting timing 2-3 times on the https://github.com/godotengine/godot/issues/93587.
@Calinou has a benchmarking server we can use too!
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.
I would recommend getting timing 2-3 times on the https://github.com/godotengine/godot/issues/93587.
Note that you can get Godot to print the time taken to import each resource by starting it with --verbose from the terminal, then looking at terminal output after each file is done importing.
Make sure to remove the .godot/ folder between each test so that everything is reimported.
- [ ] benchmarked in a production build
- [ ] benchmarked using the test cases (like linked issues)
- [ ] optional unit tests
https://github.com/zeux/meshoptimizer/pull/713
Hashing out some potential results with the owner of a potentially huge fix.
Are you reverting the reverts I did? The changes, imo, should be tested after Zeux change. I am fairly certain it would be beneficial on a file like that 700+ MB glb file called scene that we're all using. The initial file processing should be improved. (And with the Zeux change, wont take so long to test) But as it is right now, the stuff after the initial processing is the big bottleneck and this change makes no tangible difference.
4.3 has released, this is one of the pull requests I wanted to shepherd to completion.