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.

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

Core: Convert Math class to namespace #94441

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Jul 16, 2024

This adopts the formatting seen in godot-cpp which opts for Math as a namespace rather than a class. Given the class already shouldn't be instanced, nor did it really utilize any class-specific functionality, functioning as a namespace feels more apt. Beyond the conversion, the only adjustments were:

  • core/math/random_pcg.h include migrated to math_funcs.cpp; explicitly included in the handful of files utilizing RandomPCG directly.
  • default_rand, the one private field in the entire class, migrated to math_funcs.cpp as a static variable.
  • RANDOM_32BIT_MAX replaced by existing, equivalent define: UINT32_MAX.
  • static declaration removed from all functions.
  • absd, absf, is_equal_approx, and is_zero_approx moved up—a consequence of needing to be declared before use in a namespace context.
  • math_funcs.{h|cpp} functions that weren't using p_* argument syntax now do.
  • Math:: prefix removed in header where redundant.
  • double sorted before float equivalent where applicable.

@Repiteo Repiteo added this to the 4.x milestone Jul 16, 2024
@Repiteo Repiteo requested review from a team as code owners July 16, 2024 16:41
@Mickeon
Copy link
Contributor

Mickeon commented Jul 26, 2024

I assume this change qualifies as codestyle too? Or perhaps I'm missing something.

@Repiteo
Copy link
Contributor Author

Repiteo commented Jul 27, 2024

Now that you mention it, yeah; this would technically qualify as that. Given that's already the case, might as well fixup stuff like argument name syntax

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to add my two cents that although this causes churn in the code, it's probably worth doing for consistency in Godot Engine.

The pull request passes the GitHub integration tests, so it's more correct.

@Repiteo Repiteo force-pushed the core/math-namespace branch 2 times, most recently from a4070d2 to 05bcb88 Compare November 19, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants