Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve documentation of nearest_po2() #72091

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Improve documentation of nearest_po2() #72091

merged 1 commit into from
Aug 7, 2023

Conversation

MewPurPur
Copy link
Contributor

@MewPurPur MewPurPur commented Jan 26, 2023 •
edited
Loading

Note: This PR used to change behavior too, but now it's just a performance PR

Note: This PR is now just a documentation tweak.

Addresses an extra corner case for the method and words the method description a little clearer.

@MewPurPur MewPurPur requested review from a team as code owners January 26, 2023 01:05
@Calinou Calinou added this to the 4.0 milestone Jan 26, 2023
@raulsntos
Copy link
Member

It'd be great if you could also update the Mathf.NearestPo2 method in C# 🙏. 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.

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Jan 26, 2023 •
edited
Loading

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

@raulsntos
Copy link
Member

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.

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Jan 26, 2023 •
edited
Loading

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

@MewPurPur MewPurPur requested a review from a team as a code owner January 26, 2023 19:46
@MewPurPur
Copy link
Contributor Author

MewPurPur commented Feb 9, 2023 •
edited
Loading

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.

@MewPurPur MewPurPur changed the title Fix edge cases of nearest_po2() Improve performance of nearest_po2() Feb 9, 2023
@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 9, 2023
@MewPurPur
Copy link
Contributor Author

MewPurPur commented Apr 12, 2023 •
edited
Loading

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

@MewPurPur MewPurPur force-pushed the fix-nearest-po2 branch 2 times, most recently from 02b80ab to bdbc619 Compare April 12, 2023 20:52
@MewPurPur MewPurPur changed the title Improve performance of nearest_po2() Improve documentation of nearest_po2() Apr 12, 2023
@MewPurPur
Copy link
Contributor Author

pokey pokey!

@kleonc kleonc removed the performance label Aug 5, 2023
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 7, 2023
@akien-mga akien-mga merged commit 3fa5a15 into godotengine:master Aug 7, 2023
@akien-mga
Copy link
Member

Thanks!

@MewPurPur MewPurPur deleted the fix-nearest-po2 branch August 7, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kleonc kleonc kleonc approved these changes

Assignees
No one assigned
Labels
documentation enhancement
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
5 participants
@MewPurPur @raulsntos @akien-mga @kleonc @Calinou