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: Replace C math headers with C++ equivalents #104386

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

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Mar 20, 2025

One of the discussions in the Math::abs PR noted that we were still using the C API for quite a lot of our math operations, rather than C++ bindings. This PR makes the first major step to remedy this, by expunging math.h from the core entirely—in favor of cmath. It may persist via third-party includes, but ideally this will allow us to transition to the more modern syntax & better evaluate the areas of code that were using those C-headers instead of our math header.

Note that, in constrast to #96498, this simply elevates the global functions to the std namespace, NOT the Math class. Trying to integrate everything into Math would require more invasive changes, and ultimately the priority was deprecating the C-Header, so that can be addressed in a followup PR.

- Minor restructuring to ensure `math_funcs.h` is the central point for math functions
@lawnjelly
Copy link
Member

lawnjelly commented Mar 20, 2025

Most of these should be using the Godot Math:: functions I think, rather than std::.

What's the benefit of converting them to std::, and then to Math:: in another PR, versus just changing them to Math::?

@Repiteo
Copy link
Contributor Author

Repiteo commented Mar 20, 2025

If we'd rather go straight to Math::, we could just use #96498 instead. The only caveat there is it's built upon the Math namespace PR, which should be approved/merged first.

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.

2 participants