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

Adding Simple Macros To Expose Attributes to Editor More Easily #1495

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Anthony-J-G
Copy link

In my short experience of using godot-cpp and GDExtension, the pipeline has been quite smooth and easy to pick up from someone with a decent amount of C++ experience. However, there was one glaring piece that made small changes very painful and that was exposing class attributes from custom nodes to the editor.

Shown below is an example before and after of what the _bind_methods function looks like after the macros are applied to illustrate the difference they can make.

Before.
image

After.
image

With the addition of getting rid of a massive amount of getters and setters from both the source file and header file and putting them inside of macros, it creates a much simpler way to expose attributes.

Example Header File.
image

Additionally, the header file added is very non-invasive and optional so those who wish to continue using the current way of exposing attributes can do so. Note: These changes could've probably been added into include/classes/wrapped.hpp along with the GDCLASS macro definition but with the goal of being as non-invasive as possible they were added to their own header inside of include/classes/bindings.hpp.

Love GDExtension and the work you guys have done so far and look forward to more!

@Anthony-J-G Anthony-J-G requested a review from a team as a code owner June 16, 2024 03:48
@AThousandShips AThousandShips added enhancement This is an enhancement on the current functionality discussion topic:buildsystem Related to the buildsystem or CI setup labels Jun 16, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Jun 16, 2024
@AThousandShips
Copy link
Member

Please add some tests of this to make sure it works and maintain the working status, under the test directory

I'm not personally sure this kind of syntactic sugar is very useful, it only works with simple cases where the set/get is specific and named a particular way, and won't allow other patterns, which breaks the standard pattern for Boolean values in the code style for the engine (where is_ or has_ etc. is usually used)

I'd say the first part is far more useful but I'd say it should be a generic one allowing you to provide the get/set methods manually, with different versions with different details, so you can use default names if you like, and the c++ methods should be allowed to be different as well since you might want to make them private, and prefix with for example _

I think these should be highly configurable if they should be added at all, I don't think we should add macros just for very simple standard cases only


#define GDBIND(m_class, m_name, m_properties) \
const char* getter_##m_name = "get_" #m_name; \
const char* setter_##m_name = "set_" #m_name; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These shouldn't be variables IMO it pollutes and could just be as is, it doesn't make the code any simpler as you still need to write getter_##m_name that's the same as just "get_" #m_name

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah to be honest I thought the same as well but when I tried just adding it as D_METHOD("get_" #m_name) the compiler was complaining that it couldn't find the method. However, I've since changed it to something along the lines of:

#define GDBIND(m_class, m_name, m_properties, m_get_method, m_set_method) /*******************************************************/ \
::godot::ClassDB::bind_method(D_METHOD(#m_get_method), &m_class::m_get_method);	                                                    \
::godot::ClassDB::bind_method(D_METHOD(#m_set_method, "p_temp"), &m_class::m_set_method);	                                        \
::godot::ClassDB::add_property(get_class_static(), m_properties, m_set_method, m_get_method);                                       \

Was this more of the format you were thinking of?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that the method in D_METHOD and that in the class itself should be allowed to differ, that's common with some cases and naming standards (also parameters to the bound function shouldn't have a p_ prefix by the naming standards)

But it quickly becomes very complicated and fragile IMO, I personally don't like hiding relevant information that matters to the user behind a macro, it hurts readability

@dsnopek
Copy link
Collaborator

dsnopek commented Jun 17, 2024

Thanks!

While this does seem like a DX improvement, I'm not sure about adding it to godot-cpp, when Godot modules won't have access to this. One of godot-cpp's design goals is trying to make its API as close as possible to Godot's internal APIs. We're not doing a perfect job of it, but we are progressively fixing the differences, and I'm not sure we should add a whole bunch of new differences.

Perhaps this can be proposed for inclusion in Godot itself as well?

@Anthony-J-G
Copy link
Author

It could probably do well to exist inside of the engine once it is succinct and extendable enough, and allows for sufficient configuration as @AThousandShips was implying. The issue I'm seeing with that however is that it would probably end up breaking engine convention and result in a more drastic amount of changes there.

I don't know if anyone else has had the same problems that these macros try to address so perhaps it's not as applicable as I thought at first? If there are any other suggestions or ideas about how to decrease the verbosity of exposing attributes while keeping the configurability for more than just the standard case I'm totally down to make changes to comply with the design standards of godot-cpp. My original intent was based off of a mix of how the conventions worked and other codebases I've worked in where there were simple ways of exposing/binding to an external client.

Otherwise, I'm totally ok to close this and just stick to using it internally within my own modules. Would love to hear your thoughts either way!

@dsnopek
Copy link
Collaborator

dsnopek commented Jun 26, 2024

Otherwise, I'm totally ok to close this and just stick to using it internally within my own modules

You could also make it as a separate header-only library in its own repo that could be used with godot-cpp? Then you're able to share it, but don't have to worry about conforming to the design goals or standards of godot-cpp or Godot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants