-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Allow exporting variables of type Variant #89324
base: master
Are you sure you want to change the base?
Conversation
4273cf7
to
72609e6
Compare
I wanted to salvage/re-implement it. I think this should work like |
I thought about that too, but opted for a simpler solution (as the goal was customizability itself). |
72609e6
to
ca78cdf
Compare
Ok reworked JRnkfXjKv5.mp4 |
Just checking in to see if this can be approved and merged? |
We're currently in feature freeze so this won't be considered until 4.4, and it's not been decided on yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me at a glance. Could you look into rebasing this on top of master
?
PS: Is there much use in exposing Object
in the list of types in the dropdown? As far as I know, you can't give it any useful value directly in the inspector.
You can assign it a Resource. |
ca78cdf
to
e2134fb
Compare
if (export_type.is_variant() || export_type.has_no_type()) { | ||
if (is_dict) { | ||
// Dictionary allowed to have a variant key/value. | ||
export_type.kind = GDScriptParser::DataType::BUILTIN; | ||
} else { | ||
push_error(R"(Cannot use simple "@export" annotation because the type of the initialized value can't be inferred.)", p_annotation); | ||
return false; | ||
} | ||
if (export_type.has_no_type()) { | ||
push_error(R"(Cannot use simple "@export" annotation because the type of the initialized value can't be inferred.)", p_annotation); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sus change after rebase.
@@ -997,7 +997,7 @@ void TileSourceInspectorPlugin::_confirm_change_id() { | |||
} | |||
|
|||
bool TileSourceInspectorPlugin::can_handle(Object *p_object) { | |||
return p_object->is_class("TileSetAtlasSourceProxyObject") || p_object->is_class("TileSetScenesCollectionProxyObject"); | |||
return p_object && (p_object->is_class("TileSetAtlasSourceProxyObject") || p_object->is_class("TileSetScenesCollectionProxyObject")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason this crashes without null check. I did not bother investigating why, it just makes no sense. No other plugin has this problem.
Interest in this PR came up again in godotengine/godot-proposals#7016 (comment) . Just for transparency and linking. |
Since 4.4 beta 1 came out, we are feature freezed and this probably won't get to be merged in 4.4. But is there anything preventing this to be merged? That is, besides the collosal work of maintainers in evaluating the enormous ammount of pull request the godot project receives every day and deciding, for non bugfix PRs, wheter it is worth or not incorporating into the engine, and if so, deciding when doing it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
a7146ef), there's a build error:
Generating core/version_generated.gen.h ...
Generating core/version_hash.gen.cpp ...
Compiling core/version_hash.gen.cpp ...
Compiling editor/scu/scu_editor_4.gen.cpp ...
Linking Static Library core/libcore.linuxbsd.editor.x86_64.a ...
Ranlib Library core/libcore.linuxbsd.editor.x86_64.a ...
In file included from editor/scu/scu_editor_4.gen.cpp:4:
./editor/editor_properties.cpp: In member function 'void EditorPropertyVariant::_notification(int)':
./editor/editor_properties.cpp:91:30: error: 'class MenuButton' has no member named 'set_icon'
91 | change_type->set_icon(get_editor_theme_icon("Edit"));
| ^~~~~~~~
scons: *** [editor/scu/scu_editor_4.gen.linuxbsd.editor.x86_64.o] Error 1
scons: building terminated because of errors.
e2134fb
to
36c0c6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works now 🙂
(The full line of d
is @export var d: Variant = null
, it's cut off on the screenshot.)
Note that the Variant type hint must be specified explicitly, and you must not set a default value of null
for it to work as a proper Variant export where you can change the type. I think this makes sense, as this is an uncommon use case and it's better to be explicit about it.
Inspired by godotengine/godot-proposals#9269
Closes godotengine/godot-proposals#9368
This PR allows for exporting variables of type Variant. They show a property editor similar to what Array and Dictionary are using:
JRnkfXjKv5.mp4
This allows for nice flexibility, where you can change not only the variable, but also its value and allows for better customization.
example plugin for "tri-state bool"
Example usage:
godot.windows.editor.dev.x86_64_RzAIgcf3TZ.mp4
(the text is updated with
_process()
.