godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.74k stars 575 forks source link

Generate arguments for virtual functions that take `float` as taking `float` (rather than `double`) #1433

Closed dsnopek closed 2 months ago

dsnopek commented 7 months ago

While looking into issue https://github.com/godotengine/godot-cpp/issues/1350, I noticed that we were generating virtual methods that were declared to take float in Godot as taking double in godot-cpp.

For example, AudioStreamPlayback::_mix(AudioFrame *, float, int) would turn up in godot-cpp as AudioStreamPlayback::_mix(AudioFrame *, double, int). This is despite the extension_api.json giving float 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 the double * passed in the ptrcall into a float 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. :-)

fire commented 7 months ago

float means double in gdscript. I am not sure the exact way to resolve this.

AThousandShips commented 7 months ago

The meta type matches the typing of the actual c++ method though, the same is done in C#

dsnopek commented 7 months ago

Yeah, when it's a float in Godot's C++ API, GDScript's double would be converted to a float at some point anyway.

Here it's a question of how we expose Godot's C++ API's that take floats to developers using godot-cpp. I feel like they should see a float if the API uses a float in Godot's C++, since we want to match the API between Godot and godot-cpp. And this could also help avoid the possible misconception that all the precision of the double would be maintained, since it would be converted to/from a float at some point anyway.

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.

dsnopek commented 5 months ago

I spent a little more time looking at this, including diff'ing the gen/ directory both before and after this PR, to make sure the changes match expectations. Everything is looking correct to me!

Personally, I think we should make this change: it makes godot-cpp's API match the internal Godot API, using float where Godot uses float. It's possible (although, not super likely) that this could break source compatibility for some extensions, primarily for virtual methods. Most of the affected virtual methods are not commonly used, but there are some physics ones, which Jolt and the other physics extensions may hit.

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.

dsnopek commented 5 months ago

The failing CI will be fixed by PR https://github.com/godotengine/godot-cpp/pull/1486 - it's unrelated to these changes

dsnopek commented 5 months ago

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 4.1 branch) and that it only affects a small number of methods.

dsnopek commented 2 months ago

Superseded by PR #1555