-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add GDExtension compatibility methods #76577
Conversation
8053891
to
ae95c8f
Compare
Thanks, these look great! I manually tested the compatibility function for |
ae95c8f
to
7cb29cd
Compare
Discussed at the GDExtension meeting, and it looks good! Otherwise this just needs to be rebased after PR #76446 is merged, and then the remaining commits squashed into one commit, and then we'll approve it! |
7cb29cd
to
670a801
Compare
670a801
to
f0d8ac0
Compare
f0d8ac0
to
43e447b
Compare
Added more of the same, 3 more PR with similar breaking changes have been merged since. And #69988 has removed a whole bunch of APIs but I didn't do anything about that (also IIRC these apis were marked experimental before anyways) |
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.
The latest updates look good to me!
43e447b
to
5e67da6
Compare
886f675
to
eaf8bc7
Compare
205d7c5
to
f806e07
Compare
@TokageItLab Protected methods were made public, can we check the AnimationNode? Maybe they need to be private. |
Needs a rebase, by the way. |
f806e07
to
9d3c431
Compare
9d3c431
to
f4bced9
Compare
This is something I'd like @reduz to sign off on, so I think moving this to a different PR makes sense. |
f4bced9
to
4be7a81
Compare
Assuming we don't end up reverting PR #77410, we've now pretty completely broken compatibility with GDExtensions built for Godot 4.0, which makes these compatibility methods not very useful for Godot 4.1. That said, this work in general is still very useful, as it helps figure out exactly how we're going to add these going forward! And I still really support the idea of using the compatibility method system ASAP, so that we can get some valuable practice with using it. |
@dsnopek So what do you think we should do with this PR then? Reject it, but return to it post 4.1? Or merge anyway as a reference point? |
That's a good question. Maybe leave it be for a little while as we watch how things turn out after #77410? But assuming that all turns out good, I'd personally probably lean towards "Reject it, but return to it post 4.1?" It doesn't really make sense to merge code that won't ever get used, and I'm sure we'll have no shortage of compatibility methods to add after 4.1 stable is out. What do others think? |
I guess we can leave it be for now, so when we need to make a 4.1+ version, we have a starting point. I'll turn it into a draft for now. |
Closing as there are probably enough compat methods in the engine so that this is no longer needed as reference. |
Indeed, but still thanks for work and for providing everyone with a framework! |
Requires #76446.
Split into many commit to make it possible to cherry pick only the relevant bits (assuming that #76446 gets backported). I will add a commit with compat bindings for #75779, if that becomes possible.looks like that's not happening (for now)Compared to 4.0.2 this leaved one reported issue:
Which was completely useless and intentionally removed.
Compared to 4.0.1 a whole bunch of additional issues appear, that are all the hash changes that can't have compat methods atm:
And finally 4.0.0 has a few more changes where no compat code can be created:
Additionally a bunch of virtual methods have changes between 4.0 and 4.1 but they are currently not validated.Check
misc/extension_api_validation/*.expected
for up to date details.