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 PropertyListHelper #84635

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 8, 2023

Follow-up to #83888 (partially)

This PR adds PropertyListHelper. It's a helper class for managing dynamic properties added with _get_property_list(). Initialize it with prefix, then register properties with name, default, setter and getter. Afterwards, instead of implementing _get_property_list(), _set() and _get(), just call a method in the helper and it handles everything for you. As a bonus it can also handle reverts.

I added it to ItemList. Property handling is now a bunch of one-liners, while the functionality has improved. Defaults no longer save in the scene and you can revert.

godot.windows.editor.dev.x86_64_1PxuOZSe6G.mp4

I will do a follow-up to apply it to all relevant classes (TileMap, PopupMenu and probably some others).

UPDATE
In the second commit I changed how properties are registered and the 2 implementations are different. idk which one is better.

Production edit: closes godotengine/godot-roadmap#14

@sairam4123
Copy link

Could it be exposed to scripting, I think this will be of good value if it could be exposed.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 9, 2023

Not in this PR, and that would likely require a proposal. It's easier to maintain a class if it's internal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it in scene when its functionality relates to all objects, not just nodes? Although I guess we already have PropertyUtils in here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's needed pretty much only for editing these objects in the inspector. scene is both nodes and resources, so I think it's fitting.

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 9, 2023

This feels kind of awkward that some properties would now be "registered" in the constructor. I understand that this needs to happen per instance, but maybe we can figure out a way to register those dynamic indexed properties in ClassDB (so it works in _bind_methods) and resolve them dynamically?

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 9, 2023

They could be added to ClassDB, but not sure how to resolve setters/getters then. I made them use callable_mp(), because it's faster than calling them by name, and it requires instance. Maybe there is a trick to fetch the method pointer from method name, but it would involve some core changes for registering and then assigning these properties to an instance. And that's for something that's basically only useful in the editor inspector, you can't even have them documented, because these properties can't be used without knowing their full name. IMO not worth it.

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 9, 2023

IMO not worth it.

I think adding a third place to define properties is not a great idea. That makes classes harder to maintain. So anything we can do to avoid that is worth it, in my opinion.

@KoBeWi KoBeWi force-pushed the all_hail_PropertyListHelper branch from 53ad7ea to 79ab0ab Compare November 9, 2023 15:05
@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 9, 2023

I added a static PropertyListHelper that registers properties and then instances are created for actual objects. It uses regular Callables though.

@KoBeWi KoBeWi force-pushed the all_hail_PropertyListHelper branch from 79ab0ab to b59e8c7 Compare January 13, 2024 21:39
@YuriSizov YuriSizov self-requested a review January 17, 2024 12:10
@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Jan 17, 2024
@KoBeWi KoBeWi force-pushed the all_hail_PropertyListHelper branch from ba4706d to 2c14c08 Compare January 19, 2024 19:32
@@ -35,6 +35,8 @@
#include "core/string/translation.h"
#include "scene/theme/theme_db.h"

PropertyListHelper ItemList::base_property_helper;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is not a bit unnecessary. Considering that you have to implement _set, _get and _get_property_list anyway, it's clear that this it going to be dynamic. Having to create a static object, then create another object out of it is a bit complex IMO.

Maybe we should move the initialization to the constructor? Unless it hurts performance that seems like an acceptable solution.

Another solution would be to, like, find a way to define this object in _bind_methods? So that those stay close together?

Copy link
Member Author

Choose a reason for hiding this comment

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

The old version (53ad7ea#diff-8cbdaea96117950192d3baac12f8cff87b2c4598e04e5df1aae4758f5de3b09b) was being initialized in the constructor, I changed it to static approach. It's definitely cheaper this way and it's closer to how we define the properties normally.

@akien-mga akien-mga merged commit 0bda868 into godotengine:master Feb 9, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the all_hail_PropertyListHelper branch February 9, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants