Skip to content

Replace BIND_VMETHOD by new GDVIRTUAL syntax#51970

Merged
reduz merged 1 commit intogodotengine:masterfrom
reduz:implement-gdvirtuals-everywhere
Aug 22, 2021
Merged

Replace BIND_VMETHOD by new GDVIRTUAL syntax#51970
reduz merged 1 commit intogodotengine:masterfrom
reduz:implement-gdvirtuals-everywhere

Conversation

@reduz
Copy link
Copy Markdown
Member

@reduz reduz commented Aug 22, 2021

  • 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.

@reduz reduz requested a review from a team as a code owner August 22, 2021 02:06
@reduz reduz requested a review from a team August 22, 2021 02:06
@reduz reduz requested review from a team as code owners August 22, 2021 02:06
@reduz reduz requested a review from a team August 22, 2021 02:06
@reduz reduz force-pushed the implement-gdvirtuals-everywhere branch from ffd7bd4 to 91fd592 Compare August 22, 2021 02:39
@aaronfranke aaronfranke added this to the 4.0 milestone Aug 22, 2021
Comment thread doc/classes/EditorResourcePreviewGenerator.xml Outdated
Copy link
Copy Markdown
Contributor

@BastiaanOlij BastiaanOlij left a 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 :)

Comment thread doc/classes/AnimationNode.xml Outdated
@aaronfranke
Copy link
Copy Markdown
Member

@reduz Here is a version with the doc fixed: https://github.com/aaronfranke/godot/tree/implement-gdvirtuals-everywhere

Comment thread scene/gui/control.cpp Outdated
Comment thread scene/gui/control.cpp Outdated
Comment thread core/extension/extension_api_dump.cpp Outdated
Comment thread core/io/resource.cpp Outdated
Comment thread scene/resources/syntax_highlighter.cpp Outdated
Copy link
Copy Markdown
Member

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);
}

Comment thread editor/plugins/script_editor_plugin.cpp Outdated
Copy link
Copy Markdown
Member

@Paulb23 Paulb23 Aug 22, 2021

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.

Copy link
Copy Markdown
Member Author

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.

Comment thread doc/classes/CodeEdit.xml Outdated
Comment thread editor/plugins/script_editor_plugin.cpp Outdated
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

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.

@reduz reduz force-pushed the implement-gdvirtuals-everywhere branch 2 times, most recently from e861517 to 9af9da1 Compare August 22, 2021 11:15
* 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.
@reduz reduz force-pushed the implement-gdvirtuals-everywhere branch from 9af9da1 to 3682978 Compare August 22, 2021 11:25
@reduz
Copy link
Copy Markdown
Member Author

reduz commented Aug 22, 2021

Ok, documentation should be properly kept now (used this PR as a test of #51982)

@reduz reduz merged commit a73b5fa into godotengine:master Aug 22, 2021
@neikeq
Copy link
Copy Markdown
Contributor

neikeq commented Aug 22, 2021

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.

First time I hear this, I'm curious why a virtual like _notification would be implemented in a special way. Any example of such implementation?

@madalee-com
Copy link
Copy Markdown

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);
}

Mickeon added a commit to Mickeon/godot that referenced this pull request Sep 9, 2023
Reimplements the virtual method _setup_local_to_scene, lost in godotengine#51970

Also deprecates the redundant `setup_local_to_scene_requested` signal.
mandryskowski pushed a commit to mandryskowski/godot that referenced this pull request Oct 11, 2023
Reimplements the virtual method _setup_local_to_scene, lost in godotengine#51970

Also deprecates the redundant `setup_local_to_scene_requested` signal.
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