godot
godot copied to clipboard
godotengine
Improve performance of `nearest_po2()`
Note: This PR used to change behavior too, but now it's just a performance PR
Expanded nearest_po2() so that it doesn't have to do manual work figuring out GDScript's int is 64-bit. This optimization is already done in C# and makes the function a few times faster (at least 30% faster, probably a lot more in reality)
It'd be great if you could also update the Mathf.NearestPo2 method in C# :pray:. Keep in mind we use 32-bit int in C# and I don't think we want to change that to 64-bit.
Also, we should add some unit tests for this method.
@raulsntos C# already uses this PR's optimization, but keeps the edge cases. I could add if (value <= 1) { return 1; } at the beginning though, perhaps at a slight performance cost but not too relevant. Should I though? If we're fine with those edge cases, I can only keep this GDScript optimization and otherwise keep the function's return values as they are.
I would like some sort of consensus on this before proceeding with adding test cases.
I could add
if (value <= 1) { return 1; }at the beginning
Yeah, that's what I meant, if we're changing the behavior for GDScript we should also change it for C#.
I would like some sort of consensus on this before proceeding with adding test cases.
Makes total sense, I just wanted to note that this method doesn't currently have unit tests.
Also I don't really know where to add such tests. We have tests on the engine funcs, but I can't find anything for the GlobalScope. Few of the math functions in variant_utility aren't carbon copies of ones used in the engine, which are tested in math funcs. Which is why I'm not so sure about the PR--
Stripped this PR a bit and its compat-breaking aspects. Now it's just a performance boost for GDScript and documentation tweak.
Should be safe, but it's not that important. Can wait for after 4.0
p.s. Should change label from bug to performance, given this.
Weirdly enough, I don't measure the performance boost I previously did. I have no idea why, previously I measured a 30% boost when the function had extra steps. Maybe I benchmarked poorly. Or maybe something else happened that optimized it.
Anyway, I'm yet again reshaping this. Now it's just a documentation PR lol
