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

AudioFrame parameter name mismatch #86986

Closed
aiaimimi0920 opened this issue Jan 9, 2024 · 5 comments · Fixed by #87006
Closed

AudioFrame parameter name mismatch #86986

aiaimimi0920 opened this issue Jan 9, 2024 · 5 comments · Fixed by #87006

Comments

@aiaimimi0920
Copy link

Tested versions

last code : 84e205b

System information

windows 11

Issue description

I think AudioFrame parameter name mismatch,

in this code: https://github.com/godotengine/godot/blob/84e205b5a17bfe7ace96b78c410ec10aa52241d2/core/math/audio_frame.h#L55C1-L55C1

struct AudioFrame {
	//left and right samples
	float l = 0.f, r = 0.f;
}

the param name is l and r,

but in this code:

GDREGISTER_NATIVE_STRUCT(AudioFrame, "float left;float right");

GDREGISTER_NATIVE_STRUCT(AudioFrame, "float left;float right");

the param name is left and right,

I think you need to modify one of them? If necessary, I can provide PR

Steps to reproduce

null

Minimal reproduction project (MRP)

null

@akien-mga
Copy link
Member

Does this actually cause an issue using the GDExtension strucut, or is that just a code consistency issue?

@aiaimimi0920
Copy link
Author

@akien-mga
Sorry, I'm not sure if this will trigger a problem.
However, inconsistent commands are not conducive to the development process. In my understanding, the variable names of the classes provided by godot and godot cpp should be consistent

I am developing my plugin, and the code currently generated through godot cpp is
thirdparty\godot-cpp\gen\include\godot_cpp\classes\audio_frame.hpp

#ifndef GODOT_CPP_AUDIO_FRAME_HPP
#define GODOT_CPP_AUDIO_FRAME_HPP

#include <godot_cpp/core/method_ptrcall.hpp>

namespace godot {

struct AudioFrame {
	float left;
	float right;
};

GDVIRTUAL_NATIVE_PTR(AudioFrame);

} // namespace godot

#endif // ! GODOT_CPP_AUDIO_FRAME_HPP

but in godot core cpp, the code is

struct AudioFrame {
	//left and right samples
	float l = 0.f, r = 0.f;
}

Because AudioFrame is an internal type, it is not easy to test. Sorry for not providing valid test cases

@akien-mga
Copy link
Member

Thanks for the details. I do think the names should be made to match indeed, as the aim for godot-cpp is to have its code compatible/similar with C++ modules / core engine code.

IMO more explicit is better, so I would change the core AudioFrame to use left/right. I'm testing now how wide the refactoring impact would be. Technically it breaks compat for C++ modules, but that might be a better tradeoff than breaking compat for GDExtension (assuming changing the godot-cpp names would break compat).

CC @godotengine/gdextension

@AThousandShips
Copy link
Member

AThousandShips commented Jan 9, 2024

Could use a union:

union {
    float l;
    float left;
};

Unless I'm mistaken

Or even with deprecated specific:

#ifdef DISABLE_DEPRECATED
float left;
#else
union {
    float l;
    float left;
};
#endif // DISABLE_DEPRECATED

@AThousandShips
Copy link
Member

This would also fix some other parts of the code tbh, will look at a PR for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants