Skip to content
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Apr 9, 2024

While looking into issue #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. :-)

@dsnopek dsnopek added this to the 4.x milestone Apr 9, 2024
@dsnopek dsnopek requested a review from a team as a code owner April 9, 2024 22:16
@dsnopek dsnopek marked this pull request as draft April 9, 2024 22:16
@fire
Copy link
Member

fire commented Apr 10, 2024

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

@AThousandShips
Copy link
Member

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

@dsnopek
Copy link
Contributor Author

dsnopek commented Apr 10, 2024

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 dsnopek added bug This has been identified as a bug discussion labels Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants