Skip to content

Sync math defs and functions with upstream Godot#2021

Open
Togira123 wants to merge 1 commit into
godotengine:masterfrom
Togira123:sync-math
Open

Sync math defs and functions with upstream Godot#2021
Togira123 wants to merge 1 commit into
godotengine:masterfrom
Togira123:sync-math

Conversation

@Togira123

Copy link
Copy Markdown

This PR syncs math functions and definitions with upstream Godot.
Notable changes include adding constexpr to many functions and changing macro definitions to be constexpr constants in the Math namespace. Some math function bodies were changed to stay in sync with upstream godot and to allow the constexpr specifier.

@Togira123 Togira123 requested a review from a team as a code owner July 6, 2026 13:16
@Ivorforce Ivorforce added the enhancement This is an enhancement on the current functionality label Jul 6, 2026

@Ivorforce Ivorforce left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good. Am I right in assuming you've worked manually (adding constexpr to the existing bodies)?

The way we often sync looks more like "copy-paste the upstream version, then review the diff to godot-cpp-ify it again".
For example, it seems like you're skipping SQRT3 and upstream updates to smoothstep and abs.

So I think this is mergable/a clear improvement, but I wouldn't consider it to be a full sync yet.

@Togira123

Togira123 commented Jul 6, 2026

Copy link
Copy Markdown
Author

What I did was manually compare the functions to the upstream version and made changes where needed. Copy-pasting seemed a bit more unreliable here since I noticed that we have functions that upstream does not have and many functions that upstream has weren't synced in the past either, additionally functions are in a different order making reading diffs properly harder. I can still take another look and fix the remaining changes though yes :)

@Ivorforce

Copy link
Copy Markdown
Member

Sounds good!

@Togira123

Copy link
Copy Markdown
Author

Oh and another thing. I saw that our Math::sign implementation uses SIGN from defs.hpp, while min, max and clamp do not. Should I change these three to also use the respective implementations from defs.hpp for consistency?

@Ivorforce

Copy link
Copy Markdown
Member

Good question. It's interesting that godot-cpp has versions of them in the first place even though we usually try to avoid adding more features that are not in godot upstream.

I'd say yes, keep them to avoid compat breaks (and because godot might move them to Math eventually too), but use the canonical defs.hpp implementations.

@Togira123

Copy link
Copy Markdown
Author

Noticed that there is a mix of using static_cast and C-style casts in math.hpp (in places where static_cast would also be applicable). I can replace C-style casts with static_cast where possible if you want.

@Ivorforce

Copy link
Copy Markdown
Member

Whatever upstream Godot uses is fine. static_cast is indeed preferable, but the more we deviate from upstream, the harder it gets to synchronize in the future.

@Togira123

Copy link
Copy Markdown
Author

There are functions that weren't synced before. My assumption is that that's because they aren't exposed in gdscript to keep the two APIs similar. Should I continue only adding functions that are available in the latest godot version's gdscript?

@Ivorforce

Copy link
Copy Markdown
Member

I'm not aware of the history, but I'd just sync everything. The goal of godot-cpp is to match Godot's own c++ as closely as possible, not necessarily the GDScript API.

@Togira123

Copy link
Copy Markdown
Author

Alright, a lot has changed😅 comparing math_defs.hpp with upstream now it is the same except for the enums, which I saw are generated automatically, hence I didn't include them. math.hpp is also almost identical to its godot counterpart, except for a few function declarations that weren't synced before either.

@Togira123 Togira123 force-pushed the sync-math branch 4 times, most recently from accaa7e to 0748b6e Compare July 6, 2026 16:19
@Togira123

Copy link
Copy Markdown
Author

removed the int_alt_t and uint_alt_t since std::conditional_t was causing compilation errors on linux and windows. Not sure whether we need these two anyway

@Ivorforce Ivorforce left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whew, this turned quite large now, but going by the diffs it looks all good!

For posterity: Math_PI etc. macros, absf/absd, db2linear/linear2db are all deleted, and CMP_EPSILON etc. move into the godot namespace. All without no compatibility shim.
I don't think any of these are widely used, they're easy to fix, and providing no compat is defensible for 10.0. So I agree with the decision.

Thank you for taking care of this!!

Comment thread include/godot_cpp/core/math.hpp Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement This is an enhancement on the current functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants