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

Fix print_verbose() macro conflicting with UtilityFunctions::print_verbose() #1668

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Dec 9, 2024

This is regression from #1653 that I found when trying out the 'godot_openxr_vendors' extension with the latest godot-cpp:

plugin/src/main/cpp/classes/openxr_fb_render_model.cpp:84:85: error: too many arguments provided to function-like macro invocation
                UtilityFunctions::print_verbose("Failed to load render model buffer from path [", render_model_path, "] in OpenXRFbRenderModel node");
                                                                                                  ^
thirdparty/godot-cpp/include/godot_cpp/core/print_string.hpp:67:9: note: macro 'print_verbose' defined here
#define print_verbose(m_variant)          \
        ^
plugin/src/main/cpp/classes/openxr_fb_render_model.cpp:84:3: error: reference to overloaded function could not be resolved; did you mean to call it?
                UtilityFunctions::print_verbose("Failed to load render model buffer from path [", render_model_path, "] in OpenXRFbRenderModel node");
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
thirdparty/godot-cpp/gen/include/godot_cpp/variant/utility_functions.hpp:266:14: note: possible target for call
        static void print_verbose(const Variant &p_arg1, const Args &...p_args) {
                    ^
plugin/src/main/cpp/classes/openxr_fb_render_model.cpp:95:21: error: expected identifier
                UtilityFunctions::print_verbose("Failed to instance render model in OpenXRFbRenderModel node");
                                  ^
plugin/src/main/cpp/classes/openxr_fb_render_model.cpp:95:21: error: expected expression
thirdparty/godot-cpp/include/godot_cpp/core/print_string.hpp:69:3: note: expanded from macro 'print_verbose'
                if (is_print_verbose_enabled()) { \
                ^
4 errors generated.

The print_verbose() macro is conflicting with code that was already using the UtilityFunctions::print_verbose() function.

This switches to just using a normal function.

I don't know if the attempt at a performance optimization in the macro is really important, but I feel like we could look at that separately - just keeping existing code working is the most important.

@dsnopek dsnopek added bug This has been identified as a bug regression labels Dec 9, 2024
@dsnopek dsnopek added this to the 4.x milestone Dec 9, 2024
@dsnopek dsnopek requested a review from a team as a code owner December 9, 2024 17:43
@dsnopek dsnopek requested review from aaronfranke and removed request for a team December 9, 2024 17:48
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Unfortunate, but it makes sense. I'll click approve, but before merging, I wonder if a better change would be to change this to all-caps PRINT_VERBOSE in both godot-cpp and in the engine. It's weird that this macro isn't capitalized like every other macro. Making this change would require changing many lines of engine code, but I think it's for the best.

@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 9, 2024

Thanks!

I'll click approve, but before merging, I wonder if a better change would be to change this to all-caps PRINT_VERBOSE in both godot-cpp and in the engine. It's weird that this macro isn't capitalized like every other macro. Making this change would require changing many lines of engine code, but I think it's for the best.

Yeah, I'd support making that change in upstream Godot!

But I don't think we should necessarily wait on that here. It's hard to predict how long it'll take for upstream changes to be merged.

@dsnopek dsnopek merged commit 27ffd8c into godotengine:master Dec 10, 2024
11 checks passed
@dsnopek
Copy link
Collaborator Author

dsnopek commented Jan 27, 2025

Cherry-picked for 4.3 in PR #1695

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants