Skip to content

Add capitalized PRINT_VERBOSE macro alongside the lowercase one #106147

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

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

Conversation

aaronfranke
Copy link
Member

In the Godot core team meeting on 2025-05-07, it was discussed that PR #100224 is a massive codebase-wide change that will conflict with many PRs and break compatibility with third-party modules. Therefore, there is a lot of hesitation with merging it for a minor improvement, which is itself not unanimously agreed on.

However, there is still the problem of the name conflict in godot-cpp GDExtension, which currently uses a work-around of using a function instead of a macro. This means that print_verbose in godot-cpp has different behavior compared to the macro in the engine, and is objectively slower for no good reason.

This PR adds the capitalized PRINT_VERBOSE macro without changing all existing uses throughout the codebase. This allows us to gradually migrate without breaking every PR, and more importantly, allows us to add the capitalized macro to godot-cpp, which lets users get the behavior and performance benefits of the macro, while still having their code be compatible between in-engine C++ and godot-cpp GDExtension C++.

@aaronfranke aaronfranke requested a review from a team as a code owner May 7, 2025 15:58
@aaronfranke aaronfranke added this to the 4.x milestone May 7, 2025
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

For me this is an acceptable middle ground.
It's of course not super nice to have two things doing the same, but as long as we clearly communicate we prefer one over the other, this solves the godot-cpp problem we've been having.

Let's hear from more people though before we do anything as merge :)

@akien-mga
Copy link
Member

Instead of adding a redundant macro for what's mostly a technicality, I would prefer to evaluate first what options @dsnopek referred to here:

The name conflict part of the issue has been solved by #102062.

It fixes it on the Godot side. We still kind of have it on the godot-cpp side, although, we're working around it by making it a function, and maybe have a couple other options to work around it if we want print_verbose() to be a macro there too.

If godot-cpp is the main motivation for making this change, we should see if the problem can be solved in godot-cpp.

@dsnopek
Copy link
Contributor

dsnopek commented Jun 4, 2025

This means that print_verbose in godot-cpp has different behavior compared to the macro in the engine, and is objectively slower for no good reason.

I'm actually not sure that print_verbose is objectively slower - someone should measure it.

The reason I'm unsure is that crossing the GDExtension <-> Godot boundary has a cost, and both checking if verbose output is enabled and doing the print itself involve crossing that boundary. So, it may actually be faster to just cross the boundary once (calling print regardless of if verbose output is enabled or not), rather than checking first.

Although, thinking about this some more as I'm writing this, maybe we'd just need to cache if verbose output is enabled on the godot-cpp side, and then checking first would indeed be faster. Hm..

@aaronfranke aaronfranke force-pushed the print-verbose-add-caps branch from f58f5a2 to 57fc1b9 Compare June 9, 2025 01:57
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.

4 participants