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

Rename camera near and far private members to avoid conflict with Windows.h defines #87164

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

0x0ACB
Copy link
Contributor

@0x0ACB 0x0ACB commented Jan 14, 2024

Sadly Windows.h defines macros for both near and far so naming variables like this, especially in header files is causing a lot of issues. As soon as any file includes Windows.h before camera_3d.h your build will simply fail. While camera_3d.h does already include undefs for mingw builds this is insufficient as those macros are used in other parts of the windows headers as well.

This PR simply renames Camera3D::far and Camera3D::near to Camera3D::camera_far and Camera3D::camera_near to avoid this issue alltogether. Since all public methods stay the same this shouldn't cause any downstream issues.

@AThousandShips
Copy link
Member

These could alternatively be undefined with other things in typedefs.h where a lot of things windows badly defines are undefined, like min/max

@0x0ACB
Copy link
Contributor Author

0x0ACB commented Jan 14, 2024

That won't work. While you can remove the min/max defs from Windows.h with WIN32_LEAN_AND_MEAN or NOMINMAX undefining far and near has a lot of issues as these macros are used in a lot of additional headers that then won't compile at all. Renaming them is the only solution to this issue that I got to work. A lot of function definitions in windows headers are using the near and far macros.

@RandomShaper
Copy link
Member

This PR could also remove this snippets I had to add to some places precisely for the root issue being addressed here:

#ifdef MINGW_ENABLED
#undef near
#undef far
#endif

Those wouldn't be needed anymore.

@0x0ACB
Copy link
Contributor Author

0x0ACB commented Jan 15, 2024

Don't think these can simply be removed in other locations. Atleast the couple where I found this macro there were local variables called near and far . So it would require a lot more refactoring to get rid of all of those

scene/3d/camera_3d.h Outdated Show resolved Hide resolved
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Those are private members so it doesn't break compat for C++ modules.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jan 15, 2024
@akien-mga akien-mga changed the title Rename camera near and far Rename camera near and far private members to avoid conflict with Windows.h defines Jan 15, 2024
@akien-mga
Copy link
Member

Could you amend the commit message to be more explicit, e.g. like I renamed the PR? This would prevent confusion about this possibly renaming the actual exposed near and far properties. And we like commit messages starting with a capital letter ;)

Don't think these can simply be removed in other locations. Atleast the couple where I found this macro there were local variables called near and far . So it would require a lot more refactoring to get rid of all of those

Would be nice to do as a follow-up PR if you're interested.

@0x0ACB
Copy link
Contributor Author

0x0ACB commented Jan 15, 2024

Could you amend the commit message to be more explicit, e.g. like I renamed the PR? This would prevent confusion about this possibly renaming the actual exposed near and far properties. And we like commit messages starting with a capital letter ;)

Done.

Would be nice to do as a follow-up PR if you're interested.

Will do once I find time for that.

@fire fire requested review from RandomShaper and a team February 12, 2024 17:24
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.

@akien-mga This might require compatibility methods so we don't change the existing api for Camera3D. I am not very familiar with compatibility methods to guide.

@AThousandShips
Copy link
Member

These are private member variables in the c++ class, not exposed properties

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.

If they're private apis, that's fine.

@akien-mga akien-mga merged commit 5306b91 into godotengine:master Feb 12, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Jimmio92

This comment was marked as off-topic.

@0x0ACB
Copy link
Contributor Author

0x0ACB commented Mar 15, 2024

You alright mate?

This is not even about min and max macros, which by the way you can turn of via #define NOMINMAX.

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.

6 participants