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 gdextension_compat_hashes.cpp for double precision builds #88188

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 11, 2024

Fixes #86346

I also added the class and method name to the message about mapping to non-existent hashes, since that's pretty critical information for debugging this sort of issue.

cc @Bromeon

@dsnopek dsnopek added this to the 4.3 milestone Feb 11, 2024
@dsnopek dsnopek requested a review from a team as a code owner February 11, 2024 01:10
@dsnopek dsnopek added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Feb 11, 2024
@fire
Copy link
Member

fire commented Feb 12, 2024

What sort of review are we looking for? I have a pet interest in double precision.

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 12, 2024

Ah, well, this is a little tricky to review. It's easy to see that running godot --dump-extension-api with a double precision build no longer prints out the ~40 warning messages. However, actually checking that the hash mappings are correct would be pretty tedious.

I made this PR by:

  1. Compiling the engine with double precision on 6094738 (this is the commit before PR Fix method hashes with default arguments #81521 was merged, which fixed the hashing algorithm) and generating the extension_api.json
  2. Compiling the engine with double precision on master and generating the extension_api.json
  3. Processing the two files with a Python script to figure out the hash mapping and generate some code in a text file
  4. Manually going through gdextension_compat_hashes.cpp and copy-pasting the code in where needed with the addition of the #ifdef's

So, to really check it, you'd need to generate the two extension_api.json files and go through each one, which I'm not sure is something anyone would really be willing to do :-)

It may be easiest to approve/merge based on the warnings having gone away, and then we can fix any issues if users find any?

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.

Approve/merge based on the warnings having gone away, and then we can fix any issues if users find any?

Sure!

@akien-mga akien-mga merged commit fd43a7f into godotengine:master Feb 12, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
@dsnopek dsnopek deleted the gdextension-api-double-compatibility-hashes branch July 22, 2024 15:30
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.

GDExtension: double-precision builds differ in method hashes and lack compatibility
3 participants