HYDRA-843 : Add picking test + Link C++ tests with Qt for UI/viewport interaction #94
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
debloip-adsk
commented
Mar 8, 2024
Comment on lines
+114
to
+151
| # When using the qmath.h Qt header, we can run into macro redefinition issues. | ||
| # Specifically, qmath.h wants to define the math macros listed here : | ||
| # https://learn.microsoft.com/en-us/cpp/c-runtime-library/math-constants?view=msvc-170 | ||
| # To do so, it will first try to include cmath. cmath includes cstdlib, which includes math.h, | ||
| # which includes corecrt_math_defines.h, which defines the math macros. | ||
| # However, corecrt_math_defines.h is only included by math.h if _USE_MATH_DEFINES is defined. | ||
| # If _USE_MATH_DEFINES is not defined, Qt will temporarily define it itself, include cmath | ||
| # so that corecrt_math_defines.h ends up defining the math macros, and undefine it afterwards. See here : | ||
| # https://github.com/qt/qtbase/blob/85c69f023fe281b7f16e1a93e61be4432f7fef9b/src/corelib/kernel/qmath.h#L18-L28 | ||
| # If the math macros are still not defined after that, Qt will resort to defining the macros itself : | ||
| # https://github.com/qt/qtbase/blob/85c69f023fe281b7f16e1a93e61be4432f7fef9b/src/corelib/kernel/qmath.h#L188-L238 | ||
| # As such, it is guaranteed that the math macros are defined after including qmath.h. | ||
| # So what's the issue? Well, 2 things combined. First, it turns out that while cmath, cstdlib and | ||
| # corecrt_math_defines.h have header guards, math.h does not. Second, corecrt_math_defines.h does | ||
| # not check that the math macros are not defined before defining them itself. | ||
| # This can lead to the following scenario : | ||
| # 1. _USE_MATH_DEFINES is not defined. | ||
| # 2. We hit an #include <cmath> or <cstdlib>. | ||
| # 3. The cmath or cstdlib header guards get defined. | ||
| # 4. math.h gets included, but since _USE_MATH_DEFINES is not defined, corecrt_math_defines.h | ||
| # is not included, and the math macros do not get defined. | ||
| # 5. We hit an #include <qmath.h>. | ||
| # 6. Qt temporarily defines _USE_MATH_DEFINES and includes cmath. | ||
| # 7. We hit the cmath or cstdlib header guard and stop the inclusion, not reaching math.h | ||
| # and corecrt_math_defines.h. The math macros are still not defined. | ||
| # 8. Qt sees that the math macros are still not defined, and defines them itself. | ||
| # 9. _USE_MATH_DEFINES gets defined. | ||
| # 10. We hit an #include <math.h>. | ||
| # 11. Since there is no header guard for math.h, it is re-included. | ||
| # 12. As _USE_MATH_DEFINES is now defined, corecrt_math_defines.h does get included this time. | ||
| # 13. corecrt_math_defines.h defines the macros without checking if they have already been defined : | ||
| # macro redefinition -> warning -> warning treated-as-error -> error -> compilation fails. | ||
| # In our case, step 9 (_USE_MATH_DEFINES getting defined late) happens in MTypes.h, and step 10 | ||
| # (math.h re-inclusion) in some other Maya headers, for example MFloatVector.h. | ||
| # By defining _USE_MATH_DEFINES from the get-go, we ensure that the math macros get defined by | ||
| # corecrt_math_defines.h the first time around. Afterwards, even if math.h does not have a | ||
| # header guard, the one in corecrt_math_defines.h avoids redefining the macros. | ||
| _USE_MATH_DEFINES |
There was a problem hiding this comment.
This is the new fix for the error I was getting in preflights. This might only be a problem when using Qt, but I'm putting this in the general compiler_config.cmake so we don't have to re-discover this again. There's also another discussion to be had about why this error only occurred in preflights, I'll follow up on that separately.
|
Note : preflight is currently failing due to not finding LookdevX, a previous preflight did pass and the changes since then were just comments |
seando-adsk
approved these changes
Mar 11, 2024
ppt-adsk
approved these changes
Mar 11, 2024
debloip-adsk
added
ready-for-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a unit test for picking. To do so, it also adds the ability to interact with the viewport (or more largely the UI as a whole) by linking the C++ tests plugin with Qt.