-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Replace BIND_VMETHOD by new GDVIRTUAL syntax #51970
Replace BIND_VMETHOD by new GDVIRTUAL syntax #51970
Conversation
ffd7bd4
to
91fd592
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.
This looks amazing! the missing ingredient I've wanted for so long for XRInterface
, looking forward to applying it :)
@reduz Here is a version with the doc fixed: https://github.com/aaronfranke/godot/tree/implement-gdvirtuals-everywhere |
} else { | ||
color_map = _get_line_syntax_highlighting(p_line); | ||
} | ||
GDVIRTUAL_CALL(_get_line_syntax_highlighting, p_line, color_map); |
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.
This looks wrong, we're not calling _get_line_syntax_highlighting_impl
, should be something like?
if (!GDVIRTUAL_CALL(_get_line_syntax_highlighting, p_line, color_map)) {
color_map = _get_line_syntax_highlighting_impl(p_line);
}
@@ -223,8 +222,6 @@ void ScriptEditorBase::_bind_methods() { | |||
// TODO: This signal is no use for VisualScript. | |||
ADD_SIGNAL(MethodInfo("search_in_files_requested", PropertyInfo(Variant::STRING, "text"))); | |||
ADD_SIGNAL(MethodInfo("replace_in_files_requested", PropertyInfo(Variant::STRING, "text"))); | |||
|
|||
BIND_VMETHOD(MethodInfo("_add_syntax_highlighter", PropertyInfo(Variant::OBJECT, "highlighter"))); |
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.
Looks like an issue from #49546 and #49619, as it should be add_syntax_highlighter
for the virtual add_syntax_highlighter
method.
If I recall correctly, this method was only virtual bound so it would appear in the docs. as the main bound method is in the child classes, script_text_editor
here, text_editor
here and visual_script_editor
here.
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.
I could not figure out how this works, but it needs to be remade to work with the new system, if you can take a look and lend me a hand fixing it after I merge this PR I would be very grateful.
@@ -81,9 +81,8 @@ Ref<EditorSyntaxHighlighter> EditorSyntaxHighlighter::_create() const { | |||
void EditorSyntaxHighlighter::_bind_methods() { | |||
ClassDB::bind_method(D_METHOD("_get_edited_resource"), &EditorSyntaxHighlighter::_get_edited_resource); | |||
|
|||
BIND_VMETHOD(MethodInfo(Variant::STRING, "_get_name")); | |||
BIND_VMETHOD(MethodInfo(Variant::ARRAY, "_get_supported_languages")); | |||
BIND_VMETHOD(MethodInfo(Variant::ARRAY, "_get_supported_extentions")); |
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.
Need to rebind _get_supported_extentions
.
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.
I could not figure out how this works, as the way it was done did not work with the new system. The idea is to entirely get rid of BIND_VMETHOD (still need a few more PRs to achieve that) and only use the new system.
e861517
to
9af9da1
Compare
* New syntax is type safe. * New syntax allows for type safe virtuals in native extensions. * New syntax permits extremely fast calling. Note: Everything was replaced where possible except for `_gui_input` `_input` and `_unhandled_input`. These will require API rework on a separate PR as they work different than the rest of the functions. Added a new method flag METHOD_FLAG_OBJECT_CORE, used internally. Allows to not dump the core virtuals like `_notification` to the json API, since each language will implement those as it is best fits.
9af9da1
to
3682978
Compare
Ok, documentation should be properly kept now (used this PR as a test of #51982) |
First time I hear this, I'm curious why a virtual like |
I wanted to point out that the following functions, which are intended to be overridden by plugins were renamed in this PR to keep with the spatial is now 3D naming. This would be a great thing to point out in an article detailing important changes devs need to be aware of when moving to Godot 4: Godot 3 forward_spatial_gui_input
forward_spatial_draw_over_viewport
forward_spatial_force_draw_over_viewport Godot 4 _forward_3d_gui_input
_forward_3d_draw_over_viewport
_forward_3d_force_draw_over_viewport Also, the code only MOSTLY renames this, as the non-underscore functions within the base class have not been renamed. See below example where the function is named with "spatial" but calls the virtual "3d" void EditorPlugin::forward_spatial_draw_over_viewport(Control *p_overlay) {
GDVIRTUAL_CALL(_forward_3d_draw_over_viewport, p_overlay);
} |
Reimplements the virtual method _setup_local_to_scene, lost in godotengine#51970 Also deprecates the redundant `setup_local_to_scene_requested` signal.
Reimplements the virtual method _setup_local_to_scene, lost in godotengine#51970 Also deprecates the redundant `setup_local_to_scene_requested` signal.
Note: Everything was replaced where possible except for
_gui_input
_input
and_unhandled_input
.These will require API rework on a separate PR as they work different than the rest of the functions.
Added a new method flag METHOD_FLAG_OBJECT_CORE, used internally. Allows to not dump the core virtuals like
_notification
to the json API, since each language will implement those as it is best fits.