jrouwe / JoltPhysics

A multi core friendly rigid body physics and collision detection library. Written in C++. Suitable for games and VR applications. Used by Horizon Forbidden West.
MIT License
6.42k stars 414 forks source link

Adds a global variable to retrieve the list of all the preprocessor defines the binary is compiled with. #1074

Closed AndreaCatania closed 4 months ago

AndreaCatania commented 4 months ago

This variable is useful to get a human-readable string that help verify the binary settings. This is an example how it can be used (this is unrea engine code):

    verifyf(JPH::VerifyJoltVersionID(), TEXT('Preprocessor defines used by Jolt binary %s differs from the one used by this module %s.'), *FString(JPH::BinaryConfigurationString), *FString(JPH::GetConfigurationString()));
    UE_LOG(LogJolt, Log, TEXT('Jolt binary Configuration: %s'), *FString(JPH::BinaryConfigurationString));
    UE_LOG(LogJolt, Log, TEXT('Game Jolt Configuration: %s'), *FString(JPH::GetConfigurationString()));
jrouwe commented 4 months ago

The BinaryConfigurationString should technically not be needed. If this code functions correctly:

https://github.com/jrouwe/JoltPhysics/blob/1ac33c3e7caa49c80db9d78b18451898d231ab6f/Jolt/RegisterTypes.cpp#L81-L97

then you should not be able to get into a situation where these strings differ.

AndreaCatania commented 4 months ago

Ok, thanks for merging!

Regarding the function to verify the config you just linked, the instructions set configuration (avx, fmadd, etc...) is not checked.

I know we have discussed this already, but when the preprocessor defines for the instructions set differ between the two projects the build fails. If I remember well, some types even define a specific storage depending on the instructions set enables. So I wonder if they should be checked too, enforcing the two libs to have the same preprocessor defines.

jrouwe commented 4 months ago

If the build fails, then this code will never get to run right? So what point is there to have code to check it?

If the build doesn't fail (e.g. in the case of FMADD on on one side and not on the other), then there's no issue either, the classes will be binary compatible.

I'm only checking defines of which I know they will cause a binary incompatibility.

AndreaCatania commented 4 months ago

I think you are right. However, the issue is that in cases of alignment issues, like the one I've debugged with Unreal Engine, where the misalignment source is not obvious - it would have been very useful to quickly and "visually" ensure the binary config is indeed using the expected configuration and that all the preprocessor defines - on the game side - are properly defined (even the one that we are sure doesn't impact the alignment, like FMADD).

Having the global variable proposed here, to visually ensure the binary config, and the function VerifyJoltVersionIDInternal, checking even the macros that don't affect the alignment, allows you to quickly assume and trust all your defines so that you can start searching the misalignment cause elsewhere - instead of modifying the Jolt Source Code just to ensure the binary definitions are as expected.

This is not a big deal, but this is the second time I feel the lack of this functionality.

AndreaCatania commented 2 months ago

The BinaryConfigurationString should technically not be needed. If this code functions correctly:

https://github.com/jrouwe/JoltPhysics/blob/1ac33c3e7caa49c80db9d78b18451898d231ab6f/Jolt/RegisterTypes.cpp#L81-L97

then you should not be able to get into a situation where these strings differ.

Hello, I've just updated Jolt and it took me 30 min to find out the new preporcessor define needed to correctly link jolt, introduced by https://github.com/jrouwe/JoltPhysics/commit/dc3ea787223d45855987e32b8bef7f9a59f6fcd2. It would have been much faster if I could compare the two strings between what I define and what Jolt is defining. Please consider the introduction of this function allowing to retrieve the preprocessor defines used by the compiled library.

jrouwe commented 2 months ago

Did you know that I document these kind of breaking changes here:

https://github.com/jrouwe/JoltPhysics/blob/78af2562cb8526ce11e473e4165e9c57d2ff250e/Docs/APIChanges.md?plain=1#L10

Also, I added it to VerifyJoltVersionID, why didn't this trigger? If you call RegisterTypes it should have traced the mismatching define.

AndreaCatania commented 2 months ago

Did you know that I document these kind of breaking changes here:

https://github.com/jrouwe/JoltPhysics/blob/78af2562cb8526ce11e473e4165e9c57d2ff250e/Docs/APIChanges.md?plain=1#L10

Also, I added it to VerifyJoltVersionID, why didn't this trigger? If you call RegisterTypes it should have traced the mismatching define.

Thanks I'll check that file the next time I encounter this type of issues. The VerifyJoltVersionID triggered but it doesn't give you any info on "why" it failed, so it took some time to figure it out. With the proposed change, it's very easy to figure out which is the missing preprocessor define missing by comparing the string between what the jolt lib is using and what the application is using.

jrouwe commented 2 months ago

The VerifyJoltVersionID triggered but it doesn't give you any info on "why" it failed, so it took some time to figure it out.

Calling RegisterTypes will trace the exact mismatches:

https://github.com/jrouwe/JoltPhysics/blob/78af2562cb8526ce11e473e4165e9c57d2ff250e/Jolt/RegisterTypes.cpp#L83-L96

AndreaCatania commented 2 months ago

The VerifyJoltVersionID triggered but it doesn't give you any info on "why" it failed, so it took some time to figure it out.

Calling RegisterTypes will trace the exact mismatches:

https://github.com/jrouwe/JoltPhysics/blob/78af2562cb8526ce11e473e4165e9c57d2ff250e/Jolt/RegisterTypes.cpp#L83-L96

I see what the problem is. My setup does the check before calling RegisterTypes and asserts if fails. I'll refactor it. Thanks!