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

Fixing GodotCPP Compile due to Godex issues #300

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

TDCRanila
Copy link
Contributor

@TDCRanila TDCRanila commented Aug 11, 2022

Hello again,

I am looking to create a game with support of a Godot GDExtension/Addon in combination with Godex. And in order to do that with a modified engine (the addition of Godex in this case), you need to update the GodotCPP Headers. Update GodotCPP Header

I followed the instructions and updating the headers went fine. However, when compiling GodotCPP there were some Godex issues making it fail. I have attached some fixes for that in the PR.
(Windows MSVC with debug configuration using; Godot Commit: godotengine/godot@11abffb)

There are some things were I am a little unsure of, so let me know what you think. :)

  1. In the Godot source, I haven't seen 'BIND_ENUM_CONSTANT' used multiple times for the same enum but for different classes, however, looking at the generated output and the define, I don't think that could be a problem.
  2. Last thing, I noticed that the 'PHASE_FINALIZE_PROCESS' flag for the 'Phase' enum wasn't being exposed to GDScript for classes 'System' and 'ECS'. Is that on purpose or perhaps forgotten?

…ed. #

- The optional argument p'_binds' parameter from 'Object::Connect' has been deprecated and should be replaced by 'Callable.bind()' instead according to Godot-Commit(d4433ae6d3a525683ef37ea521d30b6b97a44024). Thus replacing the older signal syntax with the newer one where needed.
…fined in 'editor_world_ecs.cpp' - most likely due to godot include changes. #

- Adding proper include as a fix.
…instead of a header file resulting in mutliple defined symbols in the linking process. #
…ions in generated headers. #

- Missing exposed enums for classes 'System', 'Entity2D', 'Entity3D', and 'DynamicQuery'. (Space and Phase enumerations)
…ing with mutable keyword. #

-  In the generated "dynamic_querry.h/.cpp" files of GodotCPP - when registering the following functions to GDScript; 'with_component', 'maybe_component', and 'changed_component' in Godot - the 'mutable' parameter was conflicting with the C++ 'mutable' keyword which the compiler doesn't like.
- Changed other functions' "p_parameter" to "p_is_parameter" for consistency.
… set to false. #

- The following functions; 'with_component' 'maybe_component' 'changed_component' have the 'mutable' parameter set to false by default, so reflecting that in the exposed functions.
@AndreaCatania
Copy link
Member

This is a really great feature to have!

  1. In the Godot source, I haven't seen 'BIND_ENUM_CONSTANT' used multiple times for the same enum but for different classes, however, looking at the generated output and the define, I don't think that could be a problem.

Doing a quick search it seems like BIND_ENUM_CONSTANT is used for each enum, check this: https://github.com/godotengine/godot/blob/8f05263bd5417f1afeb46405a53a49c687b39240/servers/rendering/rendering_device.cpp#L495-L500

🤔 Did you find some duplicated constants binds? In that case I think it's an error. Please let me know

  1. Last thing, I noticed that the 'PHASE_FINALIZE_PROCESS' flag for the 'Phase' enum wasn't being exposed to GDScript for classes 'System' and 'ECS'. Is that on purpose or perhaps forgotten?

I think I just forgot to add it.

@TDCRanila
Copy link
Contributor Author

Regarding the BIND_ENUM_CONSTANT define; Correct, every enum flag that has to be exposed to GodotCPP headers has to be bound using that define.
What I find odd in the Godot source is that there are no - as far as I can see - enums bound multiple times for different classes, like what I am doing with the 'Space' enum in this commit here. (The same enum ('Space') defined in 'storage.h' used by different classes('DynamicQuery, Entity2D, Entity3D') to bind each flag.)

In the Godot source for instance, in the visual_shader_nodes header and source files, the 'Function' enum is duplicated/defined multiple times in different classes, but they are also binding the 'FUNC_MAX' flag multiple times for each class.
To me this seems like either a code style thing or it is done on purpose for some reason.

Like should this pattern be followed and should there also be separate 'Space' enums for 'DynamicQuery', 'Entity2D', and 'Entity3D'? To me this feels more like an anti-pattern with the duplicate code everywhere. And even when compiling GodotCPP, there are no differences as to how the enums are defined between the generated classes.

godot-cpp/gen/include/godot_cpp/classes/entity2d.hpp:

class Entity2D : public Node2D {
	GDNATIVE_CLASS(Entity2D, Node2D)
public:
----->enum Space { LOCAL = 0, GLOBAL = 1, };
	uint32_t get_entity_id() const;
	void add_component(const StringName &component_name, const Dictionary &values = Dictionary());
...

godot-cpp/gen/include/godot_cpp/classes/dynamic_query.hpp:

class DynamicQuery : public GodexWorldFetcher {
	GDNATIVE_CLASS(DynamicQuery, GodexWorldFetcher)
public:
----->enum Space { LOCAL = 0, GLOBAL = 1, };
	void set_space(Space space);
	void with_component(uint32_t component_id, bool is_mutable = false);
...

godot-cpp/gen/include/godot_cpp/classes/visual_shader_node_color_func.hpp

class VisualShaderNodeColorFunc : public VisualShaderNode {
	GDNATIVE_CLASS(VisualShaderNodeColorFunc, VisualShaderNode)
public:
----->enum Function { FUNC_GRAYSCALE = 0, FUNC_HSV2RGB = 1, FUNC_RGB2HSV = 2,
		FUNC_SEPIA = 3, FUNC_MAX = 4,
	};
	void set_function(VisualShaderNodeColorFunc::Function func);
....

godot-cpp/gen/include/godot_cpp/classes/visual_shader_node_vector_func.hpp

class VisualShaderNodeVectorFunc : public VisualShaderNodeVectorBase {
	GDNATIVE_CLASS(VisualShaderNodeVectorFunc, VisualShaderNodeVectorBase)
public:
----->enum Function {FUNC_NORMALIZE = 0,FUNC_SATURATE = 1,FUNC_NEGATE = 2,
		FUNC_RECIPROCAL = 3,FUNC_ABS = 4,FUNC_ACOS = 5,FUNC_ACOSH = 6,
		...
		FUNC_TANH = 30, FUNC_TRUNC = 31, FUNC_ONEMINUS = 32, FUNC_MAX = 33,
	};
	void set_function(VisualShaderNodeVectorFunc::Function func);
....

This is more of a nitpick and I think the commits are fine like they are now, however, as there is barely any documentation besides the source on this stuff like BIND_ENUM_CONSTANT , I question these things. :0

@TDCRanila TDCRanila marked this pull request as ready for review August 19, 2022 17:01
@TDCRanila
Copy link
Contributor Author

Also, I am curious, are there any automated build steps to check if GodotCPP compiles or not? I compiled it on my machine just fine (with some scripts), but that is only on Windows and such. If not, is that perhaps something to look into to make sure GDExtension support is ensured?

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

Successfully merging this pull request may close these issues.

2 participants