-
Notifications
You must be signed in to change notification settings - Fork 210
update GDNative headers and API bindings, add associated constants #207
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,53 @@ pub struct {name} {{ | |
Ok(()) | ||
} | ||
|
||
pub fn generate_class_constants(output: &mut impl Write, class: &GodotClass) -> GeneratorResult { | ||
if class.constants.is_empty() { | ||
return Ok(()); | ||
} | ||
|
||
writeln!(output, "/// Constants")?; | ||
writeln!(output, "#[allow(non_upper_case_globals)]")?; | ||
writeln!(output, "impl {} {{", class.name)?; | ||
|
||
for (name, value) in &class.constants { | ||
writeln!( | ||
output, | ||
" pub const {name}: i64 = {value};", | ||
name = name, | ||
value = value, | ||
)?; | ||
} | ||
|
||
writeln!(output, "}}")?; | ||
Ok(()) | ||
} | ||
|
||
#[derive(Copy, Clone, PartialEq)] | ||
struct EnumReference<'a> { | ||
class: &'a str, | ||
enum_name: &'a str, | ||
enum_variant: &'a str, | ||
} | ||
|
||
const ENUM_VARIANTS_TO_SKIP: &[EnumReference<'static>] = &[ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah. I see it's required for Rust enum generation. But methods still take There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The associated constants do exist as well, but most of the enums (all except the two that are special cased here) have distinct variants, so for me this is a matter of deprecated items and a single weird "Default" enum that I think should probably not be there in the first place. That said I agree with you, enums are only a subset of the associated constants and have more limitations, but when these limitations are dealt with properly they are quite nice to use (being able to do an exhaustive match for example). |
||
EnumReference { | ||
class: "MultiplayerAPI", | ||
enum_name: "RPCMode", | ||
enum_variant: "RPC_MODE_SLAVE", | ||
}, | ||
EnumReference { | ||
class: "MultiplayerAPI", | ||
enum_name: "RPCMode", | ||
enum_variant: "RPC_MODE_SYNC", | ||
}, | ||
EnumReference { | ||
class: "TextureLayered", | ||
enum_name: "Flags", | ||
enum_variant: "FLAGS_DEFAULT", | ||
}, | ||
]; | ||
|
||
pub fn generate_enum(output: &mut impl Write, class: &GodotClass, e: &Enum) -> GeneratorResult { | ||
// TODO: check whether the start of the variant name is | ||
// equal to the end of the enum name and if so don't repeat it | ||
|
@@ -43,6 +90,16 @@ pub enum {class_name}{enum_name} {{"#, | |
|
||
for &(key, val) in &values { | ||
// Use lowercase to test because of different CamelCase conventions (Msaa/MSAA, etc.). | ||
let enum_ref = EnumReference { | ||
class: class.name.as_str(), | ||
enum_name: e.name.as_str(), | ||
enum_variant: key.as_str(), | ||
}; | ||
|
||
if ENUM_VARIANTS_TO_SKIP.contains(&enum_ref) { | ||
continue; | ||
} | ||
|
||
let enum_name_without_mode = if e.name.ends_with("Mode") { | ||
e.name[0..(e.name.len() - 4)].to_lowercase() | ||
} else { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,4 +9,3 @@ library = ExtResource( 1 ) | |
|
||
[node name="Node" type="Node"] | ||
script = SubResource( 1 ) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,4 +12,3 @@ sun_energy = 16.0 | |
[resource] | ||
background_mode = 2 | ||
background_sky = SubResource( 1 ) | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why these type aliases are needed. Perhaps you wanted newtypes for type safety...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just put type aliases so the HashMap is a bit easier to read meaning into. I did not really aim for strongly typed wrappers. I could make them newtypes if you think it's worth, I just preferred this over a comment :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! I think the String should be pretty self-explanatory, but I agree that a bare
i64
can be a bit hard to make sense of.I don't think they have to be newtypes. Maybe
ConstantValue
can be an enum if you want, in case constants of other types get added in the future, but at this point it probably only adds complexity. I think it's fine either way.