Expose PropertySelector to scripting #81488

nlupugla · 2023-09-09T14:41:39Z

Exposed PropertySelector to scripting so aid addon/plugin development

Closes godotengine/godot-proposals#7635.

Todos:

Run doctools to generate documentation
Fix error Parse Error: Native class "PropertySelector" cannot be constructed as it is abstract
Test in minimal project

AThousandShips · 2023-09-09T17:20:00Z

You need to run --doctool to generate documentation for the newly exposed class, and fill in the documentation

raulsntos · 2023-09-09T15:02:13Z

editor/property_selector.cpp

@@ -575,7 +575,29 @@ void PropertySelector::set_type_filter(const Vector<Variant::Type> &p_type_filte
 	type_filter = p_type_filter;
 }

+//Variant::Type isn't exposed, so need a version that takes int


Variant::Type is exposed.

Indeed: here, and used here

The reason it is not indicated as the return type of methods like typeof is that the methods in @GlobalScope are not providing full metadata

Thanks for the review!

Ah, I see, that makes sense!

Nevertheless, I am getting an error from ClassDB::bind_method when I try to bind set_type_filter instead of set_type_filter_exposable

In template: implicit instantiation of undefined template 'GetTypeInfo<const Vector<Variant::Type> &>'

Is it maybe because Variant::Type isn't itself a Variant so it can't be implicitly converted from Vector<Variant::Type> to TypedArray<Variant::Type>?

You need to use TypedArray, Vector will only work if it's a packed case I think, as there's no direct conversion between Vector and Array, and no automatic conversion is done in the binds at all AFAIK, it just uses the built-in calling methods

I tried that first actually, and it also gave an error. I tried it again just now, and this is the error I get:

In template: no member named 'get_class_static' in 'Variant::Type'

Thanks for helping me out with this :)

Okay. Any ideas for a better comment to explain the choice? Maybe Need a version that takes int because TypedArray<Variant::Type> is not valid.

TypedArray<int> because TypedArray<Variant::Type> is not supported.

I didn't realize Variant::Type couldn't be used with TypedArray<T>. Would it be possible to make it work? If so, it'd be nice to do it before shipping this API; otherwise, we won't be able to change it later to avoid breaking compat.

My guess is that it's because Variant::Type isn't a variant, and TypedArray can only hold Variants. I could be totally wrong here though. Who would be the best person to ask about it?

Possible that it's caused by a lack of enum conversion, will look into it

raulsntos · 2023-09-09T15:04:13Z

editor/register_editor_types.cpp

@@ -174,6 +175,7 @@ void register_editor_types() {
 	GDREGISTER_CLASS(EditorResourcePicker);
 	GDREGISTER_CLASS(EditorScriptPicker);
 	GDREGISTER_ABSTRACT_CLASS(EditorUndoRedoManager);
+	GDREGISTER_CLASS(PropertySelector);


It seems we tend to prefix the exposed classes with Editor, we should consider renaming this class before exposing it.

Good idea! Would this involve doing a refactor throughout the whole codebase? I can do that, it will just add a lot more files changed to the PR.

It may be better to wait for the Editor team to offer their opinion since this is not my area. But I think if we decide to rename the class it should likely be done in this PR.

raulsntos · 2023-09-10T11:55:03Z

editor/property_selector.h

@@ -78,6 +78,7 @@ class PropertySelector : public ConfirmationDialog {
 	void select_property_from_instance(Object *p_instance, const String &p_current = "");

 	void set_type_filter(const Vector<Variant::Type> &p_type_filter);
+	void set_type_filter_exposable(const Vector<int> &p_type_filter);


I think when we need to provide another method just for the bindings we usually use the _bind suffix, but it's not too important since this name won't be part of the public API. I think we also make these methods private and therefore prefixed with an underscore (_).

as in public set_type_filter -> private _set_type_filter and public set_type_filter_exposable -> public set_type_filter_bind ?

I'd say private _set_type_filter_bind

Bind methods shouldn't be public (they are private almost always, with some exceptions that I think are mistakes)

Ah, I see. So public set_type_filter and private _set_type_filter_bind then.

Yep, it's not uniform but private methods should be prefixed by _, they should be private to keep them internal, and communicating that it is a bind method is just a good practice to avoid any confusion.

nlupugla · 2023-09-14T16:50:30Z

Superseded by #81655.

nlupugla mentioned this pull request Sep 9, 2023

Expose PropertySelector to scripting godotengine/godot-proposals#7635

Closed

Calinou added enhancement topic:editor topic:plugin labels Sep 9, 2023

Calinou added this to the 4.x milestone Sep 9, 2023

Exposed PropertySelector to scripting

06913d5

nlupugla force-pushed the expose-property-selector branch from 5e6e542 to 06913d5 Compare September 9, 2023 16:21

AThousandShips changed the title ~~Exposed PropertySelector to scripting~~ Expose PropertySelector to scripting Sep 9, 2023

raulsntos reviewed Sep 10, 2023

View reviewed changes

nlupugla mentioned this pull request Sep 14, 2023

Expose SceneTreeDialog and PropertySelector via EditorInterface #81655

Merged

nlupugla closed this Sep 14, 2023

nlupugla deleted the expose-property-selector branch September 14, 2023 16:51

YuriSizov removed this from the 4.x milestone Sep 14, 2023

YuriSizov added the archived label Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose PropertySelector to scripting #81488

Expose PropertySelector to scripting #81488

nlupugla commented Sep 9, 2023 •

edited

Loading

AThousandShips commented Sep 9, 2023

raulsntos Sep 9, 2023

AThousandShips Sep 10, 2023 •

edited

Loading

nlupugla Sep 10, 2023

AThousandShips Sep 10, 2023 •

edited

Loading

nlupugla Sep 10, 2023

nlupugla Sep 10, 2023

AThousandShips Sep 10, 2023

raulsntos Sep 10, 2023

nlupugla Sep 10, 2023

AThousandShips Sep 10, 2023

raulsntos Sep 9, 2023

nlupugla Sep 10, 2023

raulsntos Sep 10, 2023

raulsntos Sep 10, 2023

nlupugla Sep 10, 2023

AThousandShips Sep 10, 2023

nlupugla Sep 10, 2023

AThousandShips Sep 10, 2023

nlupugla commented Sep 14, 2023

Expose PropertySelector to scripting #81488

Expose PropertySelector to scripting #81488

Conversation

nlupugla commented Sep 9, 2023 • edited Loading

AThousandShips commented Sep 9, 2023

Choose a reason for hiding this comment

AThousandShips Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AThousandShips Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlupugla commented Sep 14, 2023

nlupugla commented Sep 9, 2023 •

edited

Loading

AThousandShips Sep 10, 2023 •

edited

Loading

AThousandShips Sep 10, 2023 •

edited

Loading