GodotECS / godex

Godex is a Godot Engine ECS library.
MIT License
1.22k stars 67 forks source link

Fixing GodotCPP Compile due to Godex issues #300

Open TDCRanila opened 2 years ago

TDCRanila commented 2 years ago

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?

AndreaCatania commented 2 years ago

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

:thinking: 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 commented 2 years ago

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 commented 2 years ago

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?