Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 22, 2019
Merged

update GDNative headers and API bindings, add associated constants #207

merged 4 commits into from
Sep 22, 2019

Conversation

karroffel
Copy link
Member

This PR does multiple things:

  • update the headers to he state of 3.1 (see the tag on the godot_headers repo).
  • update the api.json to the state of stable 3.1 and fix wrapper generation
  • add bindings to associated constants of a class (many constants are also enum variants, but not all)

I think from the 3.1 version on all items in the api.json file are actually sorted so that diffs are a lot cleaner and not 80000 lines of changes every time 😄

The update in the api.json also contains a fix that makes most parameters be the actual object type (for example Node) instead of falling back to Object. This means that some code might break, but this was an error to begin with.

Since this is a rather big change I would love to get some feedback from people, so if this change could be tested locally, that would be great!

cc @tom-leys, @toasteater

@karroffel karroffel added feature Adds functionality to the library c: bindings Component: GDNative bindings (mod api) labels Sep 21, 2019
@karroffel karroffel changed the title update GDNative headers and API bindings, add associated constants [draft] update GDNative headers and API bindings, add associated constants Sep 21, 2019
@karroffel karroffel changed the title [draft] update GDNative headers and API bindings, add associated constants [WIP] update GDNative headers and API bindings, add associated constants Sep 21, 2019
@karroffel
Copy link
Member Author

Oops, I forgot the proper resolution of extension-extensions, will fix ASAP

@karroffel
Copy link
Member Author

Okay, should be fixed now!

@karroffel karroffel changed the title [WIP] update GDNative headers and API bindings, add associated constants update GDNative headers and API bindings, add associated constants Sep 21, 2019
@tom-leys
Copy link
Contributor

tom-leys commented Sep 21, 2019 via email

@tom-leys
Copy link
Contributor

Hi @karroffel - I'm very happy with this.

  1. I ran scene create example, which compiled and acted as expected
  2. I ran my project, only compile issues were where I had previously called set_parent() with an Object and now should pass a Node. I fixed those.
  3. My game loads as before and acts as before. I'm not exactly using many APIs (yet) but it has no adverse affect on me.

I'll be using your branch going forward, even prior to you merging it ;)

ghost
ghost reviewed Sep 22, 2019
View reviewed changes
bindings_generator/src/classes.rs
enum_variant: &'a str,
}

const ENUM_VARIANTS_TO_SKIP: &[EnumReference<'static>] = &[
Copy link

@ghost ghost Sep 22, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Is it necessary to skip deprecated constants? It might not be very sustainable to manually maintain a list of exceptions, when there are so many classes and constants around. Maybe it's better to just leave it to Godot's documentation?

Copy link

Choose a reason for hiding this comment

The 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 i64s, so these might not be as useful as the associated constants, that don't require this uniqueness?

Copy link
Member Author

Choose a reason for hiding this comment

The 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).

@ghost
Copy link

ghost commented Sep 22, 2019

These changes look great to me! I tried it locally and didn't see any unexpected breakage in my project.

One thing to mention is the state of #183: previously as a workaround for it, the return types of VarArgs methods were manually altered in api.json. Updating the file undoes that change, so a proper fix is now required in bindings_generator. I think it should be enough to just set rust_ret_type to Variant if has_varargs is true in generate_method_impl. It shouldn't take away a lot of usability, since Objects are usually downcasted anyway.

ghost
ghost reviewed Sep 22, 2019
View reviewed changes
bindings_generator/src/api.rs
@@ -84,6 +85,9 @@ impl GodotClass {
}
}

pub type ConstantName = String;
Copy link

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...?

Copy link
Member Author

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

Copy link

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.

@karroffel
Copy link
Member Author

Thanks for the feedback you two! Seems like it's basically good to go! I will add a fix for the #183 issue either in this PR or merge this and make that fix separately (I think that's better, makes this PR a bit more self-contained).

So thanks again! 💙

@karroffel karroffel merged commit 1929641 into godot-rust:master Sep 22, 2019
This was referenced Sep 22, 2019
Closed
Closed
@ghost ghost mentioned this pull request Dec 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
Assignees
No one assigned
Labels
c: bindings Component: GDNative bindings (mod api) feature Adds functionality to the library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@karroffel @tom-leys