-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
Generate arguments for virtual functions that take float
as taking float
(rather than double
)
#1433
Generate arguments for virtual functions that take float
as taking float
(rather than double
)
#1433
Conversation
float means double in gdscript. I am not sure the exact way to resolve this. |
The meta type matches the typing of the actual c++ method though, the same is done in C# |
Yeah, when it's a Here it's a question of how we expose Godot's C++ API's that take I just need to make sure that I'm implementing this the right way, and it isn't going to causes some other problem elsewhere. |
f515a7d
to
22c8bdd
Compare
I spent a little more time looking at this, including diff'ing the Personally, I think we should make this change: it makes godot-cpp's API match the internal Godot API, using Technically, this could be cherry-picked, but let's not, because of the source compatibility situation. Given that, I personally think the source compatibility breakage is acceptable - extension authors won't need to deal with it until they upgrade to the Godot 4.3 version of godot-cpp. |
The failing CI will be fixed by PR #1486 - it's unrelated to these changes |
…`float` (rather than `double`)
22c8bdd
to
95162fa
Compare
Discussed at the GDExtension meeting: we agreed that this change makes sense, and additionally that cherry-picking would be OK, given that developers still need to update to a newer version of the older branch (ie pulling from the latest on the |
Superseded by PR #1555 |
While looking into issue #1350, I noticed that we were generating virtual methods that were declared to take
float
in Godot as takingdouble
in godot-cpp.For example,
AudioStreamPlayback::_mix(AudioFrame *, float, int)
would turn up in godot-cpp asAudioStreamPlayback::_mix(AudioFrame *, double, int)
. This is despite theextension_api.json
givingfloat
as the "meta".So, this PR makes it so that those functions actually take
float
. This fits with our general goal of providing the same API on both the Godot and godot-cpp side.I don't know if there's any problems with doing this. However, it seems like it should be fine, because our
PtrToArg<T>::convert()
should be able to change thedouble *
passed in the ptrcall into afloat
for the function call.But I'm going to mark it as draft for now, while I investigate the implications of this change.
Note: This doesn't actually fix the problem I was looking into originally. It's entirely possible that this is totally unrelated to that issue. :-)