Skip to content

Added support for parsing i32, u32, i64, and u64 constants #530

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

Merged
merged 1 commit into from
Dec 10, 2023

Conversation

TCROC
Copy link
Contributor

@TCROC TCROC commented Dec 7, 2023

Previously, the code gen would panic if it encountered constants that were out of range of i32. Now it handles those edge cases and attempts to store the value in increasingly larger data types up to u64 before it panics.

It would be nice to in the future get a report of exactly what data types the constants are. But that would likely require making changes in the godot codebase to generate us more detailed reports. For now, this is a nice alternative that prevents the codegen from panicking.

And even in the future if these improvements are made to godot, this would still be a nice fallback for cases if there are problems on godot's side.

Let me know if you want any changes on this PR :)

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-530

@TCROC TCROC closed this Dec 7, 2023
@TCROC TCROC reopened this Dec 8, 2023
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-530

@TCROC
Copy link
Contributor Author

TCROC commented Dec 8, 2023

I know you normally prefer 1 commit, but this has 2 commits. Let me know if you only want the one, but they do both have some value.

The first one has u64 support, and the second removes u64 support as u64 is currently limited by the i64 denominator. The commit with u64 can serve as a reference for when we figure out how to handle u64 constants properly.

@Bromeon Bromeon added feature Adds functionality to the library c: engine Godot classes (nodes, resources, ...) labels Dec 8, 2023
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks!

As mentioned on Discord, there are some issues with using different constants:

  1. Multiple constants of the same group can have different types.
    • It's harder to write code that can work with multiple constants, due to their type differences.
  2. If a constant's value changes, the type changes -> breaking change.
    • Although maybe different value is already considered a break?

But maybe a good compromise would just be to have 2 instead of 3 types: i32 and i64. For all existing Godot APIs, nothing should change, as they fit into i32.

Still, it would likely mean that for the GodotSteam case, you'd have one constant in the group that's a different type than all the others. Could you link to their docs and maybe source code? You copied it in Discord but didn't link to the declaration site.


I know you normally prefer 1 commit, but this has 2 commits. Let me know if you only want the one, but they do both have some value.

We can link to the 1st commit here: 2fd6e0d
GitHub will retain it for some time. I don't think it needs to be in master, so I'd appreciate squashing.

@TCROC TCROC force-pushed the const-out-of-range-fix branch 3 times, most recently from 61fdd8d to 79c9649 Compare December 9, 2023 14:24
@TCROC
Copy link
Contributor Author

TCROC commented Dec 9, 2023

But maybe a good compromise would just be to have 2 instead of 3 types: i32 and i64. For all existing Godot APIs, nothing should change, as they fit into i32.

I agree. I think that helps simplify things.

Still, it would likely mean that for the GodotSteam case, you'd have one constant in the group that's a different type than all the others. Could you link to their docs and maybe source code? You copied it in Discord but didn't link to the declaration site.

Yep. Let me see if I can find those...

Here we go! :)

https://github.com/CoaguCo-Industries/GodotSteam/blob/godot4/godotsteam_constants.h

This is where the constants are defined that gdext uses. But they are actually referencing constants that are defined directly in the steam sdk I believe? And those aren't open source so you will have to download the sdk from here to see the values:

https://partner.steamgames.com/downloads/list

@TCROC TCROC force-pushed the const-out-of-range-fix branch from 79c9649 to 2ec1c96 Compare December 9, 2023 14:37
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks, also for the doc links!

I think ConstantValue might have been slightly clearer than ConstValue, but it's fine for now 🙂

@Bromeon Bromeon added this pull request to the merge queue Dec 10, 2023
Merged via the queue into godot-rust:master with commit c21ef9f Dec 10, 2023
@TCROC
Copy link
Contributor Author

TCROC commented Dec 10, 2023

No problem! :) and ya I can change it to ConstantValue with a new PR. I didn't even realize I shortened it. My brain autocompleted it for me since I'm so used to using the const keyword 😅

@Bromeon
Copy link
Member

Bromeon commented Dec 10, 2023

@TCROC no need to, I'll include this in some other changes of mine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: engine Godot classes (nodes, resources, ...) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants