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

Add variable definition for enum types #445

Merged
merged 15 commits into from
Apr 16, 2023
Merged

Conversation

Dakror
Copy link
Contributor

@Dakror Dakror commented Apr 10, 2023

I got tired of having to register custom getter and setters for every enum type i have in my structs, so i made this small utility change using std::underlying_type

Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks useful to me!

Include/RmlUi/Core/DataVariable.h Outdated Show resolved Hide resolved
Include/RmlUi/Core/DataTypeRegister.h Outdated Show resolved Hide resolved
Include/RmlUi/Core/DataTypeRegister.h Outdated Show resolved Hide resolved
@mikke89 mikke89 added enhancement New feature or request data binding labels Apr 10, 2023
@mikke89
Copy link
Owner

mikke89 commented Apr 12, 2023

I like this approach. A getter would be nice too, so we could use it for retrieving property values. Currently we set all enums using int as the type on properties, not sure if this could cause any issues.

I hope the library-wide formatting I decided to do isn't causing you too much trouble here. At least now you should be able to enable "format on save" if you have clang-format enabled on your editor.

There is also a large file here I don't think you intended to add.

@Dakror
Copy link
Contributor Author

Dakror commented Apr 12, 2023

oops yeah the compilation database..

@Dakror
Copy link
Contributor Author

Dakror commented Apr 12, 2023

I actually dont know what the best way for a setter would be, i dont want to be too invasive in the variant type. do you have an idea for it?

@mikke89
Copy link
Owner

mikke89 commented Apr 14, 2023

I think your solution there already is good. Maybe, to be more generic, cast it to a specific integer type (int64_t?) before storing. Otherwise, if we don't support a specific underlying type, I suspect it won't compile. static_assert the size to be sure it fits in the int64_t. I think this should be safe?

Did you mean the getter? I think the same principle, we would need an SFINAE for GetInto enums. Maybe it is a good idea to measure compile times before and after this change :)

@Dakror
Copy link
Contributor Author

Dakror commented Apr 14, 2023

image
im kinda stuck, dont know what im doing wrong in my getter... ideas?

@Dakror
Copy link
Contributor Author

Dakror commented Apr 15, 2023

I worked around it using reinterpret_cast, not sure if thats a good idea though.

@mikke89
Copy link
Owner

mikke89 commented Apr 15, 2023

I'm worried about using the type converter, because every time we use a new enum, there will be new type converter function invoked for each one of the other types in GetInto. This will quickly explode. In release mode, hopefully the linker will deduplicate the massive amount of functions that will be instantiated, but I'm worried compile times and debug mode will suffer.

I drafted something, partly based on your solution, for what I am thinking here: 7f34305

Just a small FYI, is_enum_v is a C++17 feature, we will need to use the more verbose is_enum::value.

@Dakror
Copy link
Contributor Author

Dakror commented Apr 15, 2023

True thats a good point. Thanks for sharing your solution, i kinda figured using a temporary value for the GetInto function would be a version that could work.

Think we can just use your version then

@mikke89
Copy link
Owner

mikke89 commented Apr 15, 2023

Alright, sounds good. Do you want to integrate that solution into your other changes here?

@mikke89
Copy link
Owner

mikke89 commented Apr 15, 2023

I wanted a few more changes and just went ahead and committed them, hope that's okay. If you are happy with this then I am ready to merge it.

@Dakror
Copy link
Contributor Author

Dakror commented Apr 16, 2023

looks good to me

@mikke89 mikke89 merged commit 33dd587 into mikke89:master Apr 16, 2023
@mikke89
Copy link
Owner

mikke89 commented Apr 16, 2023

Alright, thanks again for the PR.

@Dakror Dakror deleted the enum-types branch April 16, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data binding enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants