godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.13k stars 90 forks source link

Forbid access to internal, private, non-virtual C++ methods and callbacks via script #2285

Open Xrayez opened 3 years ago

Xrayez commented 3 years ago

Describe the project you are working on

Goost Godot Engine Extension.

Describe the problem or limitation you are having in your project

If you look at reports such as:

you'll see that those methods are not part of public API, yet it's still possible to call those methods and callbacks manually via script (autocompletion also won't work for those methods).

For instance, some time ago I've accidentally overridden _update_callback() in Node2D to schedule world update:

extends Node2D

var res: Resource
var update_queued = false

func _ready():
    res.connect("changed", self, "queue_update")

func queue_update():
    if update_queued:
        return
    update_queued = true
    call_deferred("_update_callback")

func _update_callback(): # some heavy processing
    update_queued = false
    update()

func _draw(): # some heavy drawing
    pass

As it turns out, _update_callback uses a similar mechanism to schedule draw calls to update(), but since I'm overriding it, nothing is actually drawn, and I couldn't figure out why until I renamed _update_callback to update_callback.

Here's another example: godotengine/godot#42755.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I'm not saying that those methods should be exposed, but I think there should be a clear and intuitive mechanism to either completely forbid the access of those methods via script, or allow to show a warning when such methods get used/overridden.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I've previously submitted a proof-of-concept PR which helps to alleviate this issue by showing a warning for those internal methods: godotengine/godot#42968.

The problem is that it's still possible to call those methods without seeing the warning (only works for methods which get overridden).

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, there's no apparent mechanism which could allow to detect those internal callbacks via script.

Is there a reason why this should be core and not an add-on in the asset library?

Can only be resolved in core.

me2beats commented 3 years ago

I agree that it was helpful in this case.

However on the other hand, if we disable it all at once, then we would lose advantages of using undocumented methods available from gdscript especially when making plugins.

For example, there are many useful ScriptEditor methods like _menu_option() get_editor_interface().get_script_editor()._menu_option(7) is for saving all scripts an script list.

I understand that it is risky to rely on such methods, but often this is the only way to solve a certain problem. And it may take a while to wait for the desired method to be exposed back.

So I am for careful manual selecting of methods that should be hidden and those that could be exposed.

Xrayez commented 3 years ago

Yes, for those we'd simply have to provide additional public API, but yeah I totally understand the problem.

Given that Godot's development philosophy is based on extreme pragmatism, oftentimes it takes a while for something to be exposed in Godot until a use case pops up to justify an addition (especially for less frequently used features), even if it makes perfect sense to expose something for completeness sake in the first place, regardless whether something has real-life usage.

I believe most features in Godot will find a use case in real-life projects anyways, the only problem I see with Godot development is maintenance, yet it shouldn't really be a problem given Godot is advertised to be a community-driven project, and I hope it is. 🙂

P.S. Sorry to talk about this from a fundamental point of view (which may seem like off-topic), but this is just where most problems can be identified and eliminated with the engine development.

qarmin commented 3 years ago

Simple script shows that there is 671 methods which starts with _ - some like _ready are proper but some are not

func _ready() -> void:
    var hidden_methods : Array = []
    for name_of_class in ClassDB.get_class_list():
        for method_data in ClassDB.class_get_method_list(name_of_class,true):
            var method_name = method_data["name"]

            if !method_name.begins_with("_"): # Non hidden method
                continue

            hidden_methods.append(name_of_class + "." + method_name)

    hidden_methods.sort()
    print("------ Found " + str(hidden_methods.size()) + " hidden methods")
    for i in hidden_methods:
        print(i)

List of all functions

``` ARVRPositionalTracker._set_joy_id ARVRPositionalTracker._set_mesh ARVRPositionalTracker._set_name ARVRPositionalTracker._set_orientation ARVRPositionalTracker._set_rw_position ARVRPositionalTracker._set_type AStar._compute_cost AStar._estimate_cost AStar2D._compute_cost AStar2D._estimate_cost AcceptDialog._builtin_text_entered AcceptDialog._custom_action AcceptDialog._ok AnimatedSprite._is_playing AnimatedSprite._res_changed AnimatedSprite._set_playing AnimatedSprite3D._is_playing AnimatedSprite3D._res_changed AnimatedSprite3D._set_playing AnimatedTexture._update_proxy AnimationNode._get_filters AnimationNode._set_filters AnimationNodeBlendSpace1D._add_blend_point AnimationNodeBlendSpace1D._tree_changed AnimationNodeBlendSpace2D._add_blend_point AnimationNodeBlendSpace2D._get_triangles AnimationNodeBlendSpace2D._set_triangles AnimationNodeBlendSpace2D._tree_changed AnimationNodeBlendSpace2D._update_triangles AnimationNodeBlendTree._node_changed AnimationNodeBlendTree._tree_changed AnimationNodeStateMachine._tree_changed AnimationPlayer._animation_changed AnimationPlayer._node_removed AnimationTree._clear_caches AnimationTree._node_removed AnimationTree._tree_changed AnimationTree._update_properties Area._area_enter_tree Area._area_exit_tree Area._area_inout Area._body_enter_tree Area._body_exit_tree Area._body_inout Area2D._area_enter_tree Area2D._area_exit_tree Area2D._area_inout Area2D._body_enter_tree Area2D._body_exit_tree Area2D._body_inout AudioStreamPlayer._bus_layout_changed AudioStreamPlayer._is_active AudioStreamPlayer._set_playing AudioStreamPlayer2D._bus_layout_changed AudioStreamPlayer2D._is_active AudioStreamPlayer2D._set_playing AudioStreamPlayer3D._bus_layout_changed AudioStreamPlayer3D._is_active AudioStreamPlayer3D._set_playing BakedLightmapData._get_user_data BakedLightmapData._set_user_data BaseButton._gui_input BaseButton._pressed BaseButton._toggled BaseButton._unhandled_input BitMap._get_data BitMap._set_data BitmapFont._get_chars BitmapFont._get_kernings BitmapFont._get_textures BitmapFont._set_chars BitmapFont._set_kernings BitmapFont._set_textures CPUParticles._update_render_thread CPUParticles2D._texture_changed CPUParticles2D._update_render_thread CSGMesh._mesh_changed CSGPolygon._has_editable_3d_polygon_no_depth CSGPolygon._is_editable_3d_polygon CSGPolygon._path_changed CSGPolygon._path_exited CSGShape._update_shape Camera2D._make_current Camera2D._set_current Camera2D._update_scroll CameraFeed._allocate_texture CameraFeed._set_RGB_img CameraFeed._set_YCbCr_img CameraFeed._set_YCbCr_imgs CameraFeed._set_name CameraFeed._set_position CanvasItem._draw CanvasItem._edit_get_pivot CanvasItem._edit_get_position CanvasItem._edit_get_rect CanvasItem._edit_get_rotation CanvasItem._edit_get_scale CanvasItem._edit_get_state CanvasItem._edit_get_transform CanvasItem._edit_set_pivot CanvasItem._edit_set_position CanvasItem._edit_set_rect CanvasItem._edit_set_rotation CanvasItem._edit_set_scale CanvasItem._edit_set_state CanvasItem._edit_use_pivot CanvasItem._edit_use_rect CanvasItem._edit_use_rotation CanvasItem._is_on_top CanvasItem._set_on_top CanvasItem._toplevel_raise_self CanvasItem._update_callback CollisionObject._input_event CollisionObject._update_debug_shapes CollisionObject2D._input_event CollisionPolygon._is_editable_3d_polygon CollisionShape._shape_changed CollisionShape2D._shape_changed ColorPicker._add_preset_pressed ColorPicker._focus_enter ColorPicker._focus_exit ColorPicker._hsv_draw ColorPicker._html_entered ColorPicker._html_focus_exit ColorPicker._preset_input ColorPicker._sample_draw ColorPicker._screen_input ColorPicker._screen_pick_pressed ColorPicker._text_type_toggled ColorPicker._update_presets ColorPicker._uv_input ColorPicker._value_changed ColorPicker._w_input ColorPickerButton._color_changed ColorPickerButton._modal_closed ConeTwistJoint._get_swing_span ConeTwistJoint._get_twist_span ConeTwistJoint._set_swing_span ConeTwistJoint._set_twist_span Container._child_minsize_changed Container._sort_children Control._clips_input Control._get_minimum_size Control._get_tooltip Control._gui_input Control._make_custom_tooltip Control._override_changed Control._set_anchor Control._set_global_position Control._set_position Control._set_size Control._size_changed Control._theme_changed Control._update_minimum_size Curve._get_data Curve._set_data Curve2D._get_data Curve2D._set_data Curve3D._get_data Curve3D._set_data CurveTexture._update EditorExportPlugin._export_begin EditorExportPlugin._export_end EditorExportPlugin._export_file EditorFileDialog._action_pressed EditorFileDialog._cancel_pressed EditorFileDialog._dir_entered EditorFileDialog._favorite_move_down EditorFileDialog._favorite_move_up EditorFileDialog._favorite_pressed EditorFileDialog._favorite_selected EditorFileDialog._file_entered EditorFileDialog._filter_selected EditorFileDialog._go_back EditorFileDialog._go_forward EditorFileDialog._go_up EditorFileDialog._item_db_selected EditorFileDialog._item_list_item_rmb_selected EditorFileDialog._item_list_rmb_clicked EditorFileDialog._item_menu_id_pressed EditorFileDialog._item_selected EditorFileDialog._items_clear_selection EditorFileDialog._make_dir EditorFileDialog._make_dir_confirm EditorFileDialog._multi_selected EditorFileDialog._recent_selected EditorFileDialog._save_confirm_pressed EditorFileDialog._select_drive EditorFileDialog._thumbnail_done EditorFileDialog._thumbnail_result EditorFileDialog._unhandled_input EditorFileDialog._update_dir EditorFileDialog._update_file_list EditorFileDialog._update_file_name EditorInspector._edit_request_change EditorInspector._feature_profile_changed EditorInspector._filter_changed EditorInspector._multiple_properties_changed EditorInspector._node_removed EditorInspector._object_id_selected EditorInspector._property_changed EditorInspector._property_changed_update_all EditorInspector._property_checked EditorInspector._property_keyed EditorInspector._property_keyed_with_value EditorInspector._property_selected EditorInspector._resource_selected EditorInspector._vscroll_changed EditorProperty._focusable_focused EditorProperty._gui_input EditorResourceConversionPlugin._convert EditorResourceConversionPlugin._converts_to EditorResourcePreview._preview_ready EditorSceneImporter._get_extensions EditorSceneImporter._get_import_flags EditorSceneImporter._import_animation EditorSceneImporter._import_scene EditorScript._run EditorSelection._emit_change EditorSelection._node_removed EditorSpinSlider._grabber_gui_input EditorSpinSlider._grabber_mouse_entered EditorSpinSlider._grabber_mouse_exited EditorSpinSlider._gui_input EditorSpinSlider._value_focus_exited EditorSpinSlider._value_input_closed EditorSpinSlider._value_input_entered EditorVCSInterface._commit EditorVCSInterface._get_file_diff EditorVCSInterface._get_modified_files_data EditorVCSInterface._get_project_name EditorVCSInterface._get_vcs_name EditorVCSInterface._initialize EditorVCSInterface._is_vcs_initialized EditorVCSInterface._shut_down EditorVCSInterface._stage_file EditorVCSInterface._unstage_file FileDialog._action_pressed FileDialog._cancel_pressed FileDialog._dir_entered FileDialog._file_entered FileDialog._filter_selected FileDialog._go_up FileDialog._make_dir FileDialog._make_dir_confirm FileDialog._save_confirm_pressed FileDialog._select_drive FileDialog._tree_item_activated FileDialog._tree_multi_selected FileDialog._tree_selected FileDialog._unhandled_input FileDialog._update_dir FileDialog._update_file_list FileDialog._update_file_name FileSystemDock._bw_history FileSystemDock._duplicate_operation_confirm FileSystemDock._feature_profile_changed FileSystemDock._file_list_activate_file FileSystemDock._file_list_gui_input FileSystemDock._file_list_rmb_option FileSystemDock._file_list_rmb_pressed FileSystemDock._file_list_rmb_select FileSystemDock._file_list_thumbnail_done FileSystemDock._file_multi_selected FileSystemDock._file_removed FileSystemDock._folder_removed FileSystemDock._fs_changed FileSystemDock._fw_history FileSystemDock._make_dir_confirm FileSystemDock._make_scene_confirm FileSystemDock._move_operation_confirm FileSystemDock._move_with_overwrite FileSystemDock._navigate_to_path FileSystemDock._preview_invalidated FileSystemDock._rename_operation_confirm FileSystemDock._rescan FileSystemDock._resource_created FileSystemDock._search_changed FileSystemDock._select_file FileSystemDock._toggle_file_display FileSystemDock._toggle_split_mode FileSystemDock._tree_activate_file FileSystemDock._tree_empty_selected FileSystemDock._tree_gui_input FileSystemDock._tree_multi_selected FileSystemDock._tree_rmb_empty FileSystemDock._tree_rmb_option FileSystemDock._tree_rmb_select FileSystemDock._tree_thumbnail_done FileSystemDock._update_import_dock FileSystemDock._update_tree GDScriptFunctionState._signal_callback Generic6DOFJoint._get_angular_hi_limit_x Generic6DOFJoint._get_angular_hi_limit_y Generic6DOFJoint._get_angular_hi_limit_z Generic6DOFJoint._get_angular_lo_limit_x Generic6DOFJoint._get_angular_lo_limit_y Generic6DOFJoint._get_angular_lo_limit_z Generic6DOFJoint._set_angular_hi_limit_x Generic6DOFJoint._set_angular_hi_limit_y Generic6DOFJoint._set_angular_hi_limit_z Generic6DOFJoint._set_angular_lo_limit_x Generic6DOFJoint._set_angular_lo_limit_y Generic6DOFJoint._set_angular_lo_limit_z GradientTexture._update GraphEdit._connections_layer_draw GraphEdit._graph_node_moved GraphEdit._graph_node_raised GraphEdit._graph_node_slot_updated GraphEdit._gui_input GraphEdit._minimap_draw GraphEdit._minimap_toggled GraphEdit._scroll_moved GraphEdit._snap_toggled GraphEdit._snap_value_changed GraphEdit._top_layer_draw GraphEdit._top_layer_input GraphEdit._update_scroll_offset GraphEdit._zoom_minus GraphEdit._zoom_plus GraphEdit._zoom_reset GraphNode._gui_input GridMap._update_octants_callback HTTPRequest._redirect_request HTTPRequest._request_done HTTPRequest._timeout HingeJoint._get_lower_limit HingeJoint._get_upper_limit HingeJoint._set_lower_limit HingeJoint._set_upper_limit Image._get_data Image._set_data ImageTexture._reload_hook ItemList._get_items ItemList._gui_input ItemList._scroll_changed ItemList._set_items Joint._body_exit_tree Joint2D._body_exit_tree KinematicBody2D._direct_state_changed LargeTexture._get_data LargeTexture._set_data LightOccluder2D._poly_changed Line2D._curve_changed Line2D._gradient_changed LineEdit._editor_settings_changed LineEdit._gui_input LineEdit._text_changed LineEdit._toggle_draw_caret MainLoop._drop_files MainLoop._finalize MainLoop._global_menu_action MainLoop._idle MainLoop._initialize MainLoop._input_event MainLoop._input_text MainLoop._iteration MenuButton._get_items MenuButton._set_items MenuButton._unhandled_key_input MeshInstance._mesh_changed MeshInstance._update_skinning MultiMesh._get_color_array MultiMesh._get_custom_data_array MultiMesh._get_transform_2d_array MultiMesh._get_transform_array MultiMesh._set_color_array MultiMesh._set_custom_data_array MultiMesh._set_transform_2d_array MultiMesh._set_transform_array MultiplayerAPI._add_peer MultiplayerAPI._connected_to_server MultiplayerAPI._connection_failed MultiplayerAPI._del_peer MultiplayerAPI._server_disconnected NavigationMesh._get_polygons NavigationMesh._set_polygons NavigationPolygon._get_outlines NavigationPolygon._get_polygons NavigationPolygon._set_outlines NavigationPolygon._set_polygons NavigationPolygonInstance._navpoly_changed Node._enter_tree Node._exit_tree Node._get_configuration_warning Node._get_editor_description Node._get_import_path Node._input Node._physics_process Node._process Node._ready Node._set_editor_description Node._set_import_path Node._unhandled_input Node._unhandled_key_input NoiseTexture._generate_texture NoiseTexture._queue_update NoiseTexture._thread_done NoiseTexture._update_texture Object._get Object._get_property_list Object._init Object._notification Object._set Object._to_string OptionButton._focused OptionButton._get_items OptionButton._select_int OptionButton._selected OptionButton._set_items PackedDataContainer._get_data PackedDataContainer._iter_get PackedDataContainer._iter_init PackedDataContainer._iter_next PackedDataContainer._set_data PackedDataContainerRef._is_dictionary PackedDataContainerRef._iter_get PackedDataContainerRef._iter_init PackedDataContainerRef._iter_next PackedScene._get_bundled_scene PackedScene._set_bundled_scene ParallaxBackground._camera_moved Path._curve_changed Path2D._curve_changed PhysicalBone._direct_state_changed PhysicsBody._get_layers PhysicsBody._set_layers PhysicsBody2D._get_layers PhysicsBody2D._set_layers Polygon2D._get_bones Polygon2D._set_bones Polygon2D._skeleton_bone_setup_changed PolygonPathFinder._get_data PolygonPathFinder._set_data PopupMenu._get_items PopupMenu._gui_input PopupMenu._set_items PopupMenu._submenu_timeout Position2D._get_gizmo_extents Position2D._set_gizmo_extents PrimitiveMesh._update ProceduralSky._thread_done ProceduralSky._update_sky ProximityGroup._proximity_group_broadcast Resource._setup_local_to_scene ResourcePreloader._get_resources ResourcePreloader._set_resources RichTextEffect._process_custom_fx RichTextLabel._gui_input RichTextLabel._scroll_changed RigidBody._body_enter_tree RigidBody._body_exit_tree RigidBody._direct_state_changed RigidBody._integrate_forces RigidBody._reload_physics_characteristics RigidBody2D._body_enter_tree RigidBody2D._body_exit_tree RigidBody2D._direct_state_changed RigidBody2D._integrate_forces RigidBody2D._reload_physics_characteristics SceneTree._change_scene SceneTree._connected_to_server SceneTree._connection_failed SceneTree._network_peer_connected SceneTree._network_peer_disconnected SceneTree._server_disconnected ScriptCreateDialog._browse_class_in_tree ScriptCreateDialog._browse_path ScriptCreateDialog._built_in_pressed ScriptCreateDialog._class_name_changed ScriptCreateDialog._create ScriptCreateDialog._file_selected ScriptCreateDialog._lang_changed ScriptCreateDialog._parent_name_changed ScriptCreateDialog._path_changed ScriptCreateDialog._path_entered ScriptCreateDialog._path_hbox_sorted ScriptCreateDialog._template_changed ScriptEditor._add_callback ScriptEditor._autosave_scripts ScriptEditor._breaked ScriptEditor._clear_execution ScriptEditor._close_all_tabs ScriptEditor._close_current_tab ScriptEditor._close_discard_current_tab ScriptEditor._close_docs_tab ScriptEditor._close_other_tabs ScriptEditor._copy_script_path ScriptEditor._editor_pause ScriptEditor._editor_play ScriptEditor._editor_settings_changed ScriptEditor._editor_stop ScriptEditor._file_dialog_action ScriptEditor._filter_methods_text_changed ScriptEditor._filter_scripts_text_changed ScriptEditor._get_debug_tooltip ScriptEditor._goto_script_line ScriptEditor._goto_script_line2 ScriptEditor._help_class_goto ScriptEditor._help_class_open ScriptEditor._help_overview_selected ScriptEditor._help_search ScriptEditor._history_back ScriptEditor._history_forward ScriptEditor._live_auto_reload_running_scripts ScriptEditor._members_overview_selected ScriptEditor._menu_option ScriptEditor._on_find_in_files_modified_files ScriptEditor._on_find_in_files_requested ScriptEditor._on_find_in_files_result_selected ScriptEditor._open_recent_script ScriptEditor._reload_scripts ScriptEditor._request_help ScriptEditor._res_saved_callback ScriptEditor._resave_scripts ScriptEditor._save_history ScriptEditor._script_changed ScriptEditor._script_created ScriptEditor._script_list_gui_input ScriptEditor._script_selected ScriptEditor._script_split_dragged ScriptEditor._set_execution ScriptEditor._show_debugger ScriptEditor._start_find_in_files ScriptEditor._tab_changed ScriptEditor._theme_option ScriptEditor._toggle_members_overview_alpha_sort ScriptEditor._tree_changed ScriptEditor._unhandled_input ScriptEditor._update_autosave_timer ScriptEditor._update_members_overview ScriptEditor._update_recent_scripts ScriptEditor._update_script_connections ScriptEditor._update_script_names ScrollBar._drag_node_exit ScrollBar._drag_node_input ScrollBar._gui_input ScrollContainer._ensure_focused_visible ScrollContainer._gui_input ScrollContainer._scroll_moved ScrollContainer._update_scrollbar_position ShaderMaterial._shader_changed Skeleton2D._update_bone_setup Skeleton2D._update_transform SkinReference._skin_changed Slider._gui_input SliderJoint._get_lower_limit_angular SliderJoint._get_upper_limit_angular SliderJoint._set_lower_limit_angular SliderJoint._set_upper_limit_angular SoftBody._draw_soft_mesh Spatial._update_gizmo SpinBox._gui_input SpinBox._line_edit_focus_exit SpinBox._line_edit_input SpinBox._range_click_timeout SpinBox._text_entered SplitContainer._gui_input Sprite._texture_changed SpriteBase3D._im_update SpriteBase3D._queue_update SpriteFrames._get_animations SpriteFrames._get_frames SpriteFrames._set_animations SpriteFrames._set_frames StaticBody._reload_physics_characteristics StaticBody2D._reload_physics_characteristics TabContainer._child_renamed_callback TabContainer._gui_input TabContainer._on_mouse_exited TabContainer._on_theme_changed TabContainer._update_current_tab Tabs._gui_input Tabs._on_mouse_exited Tabs._update_hover TextEdit._click_selection_held TextEdit._cursor_changed_emit TextEdit._gui_input TextEdit._push_current_op TextEdit._scroll_moved TextEdit._text_changed_emit TextEdit._toggle_draw_caret TextEdit._update_wrap_at TextEdit._v_scroll_input TextureLayered._get_data TextureLayered._set_data TextureRect._texture_changed Theme._emit_theme_changed TileMap._clear_quadrants TileMap._get_old_cell_size TileMap._get_tile_data TileMap._recreate_quadrants TileMap._set_celld TileMap._set_old_cell_size TileMap._set_tile_data TileSet._forward_atlas_subtile_selection TileSet._forward_subtile_selection TileSet._is_tile_bound TouchScreenButton._input Translation._get_messages Translation._set_messages Tree._gui_input Tree._popup_select Tree._range_click_timeout Tree._scroll_moved Tree._text_editor_enter Tree._text_editor_modal_close Tree._value_editor_changed Tween._remove_by_uid Viewport._gui_remove_focus Viewport._gui_show_tooltip Viewport._own_world_changed Viewport._post_gui_grab_click_focus Viewport._process_picking Viewport._subwindow_visibility_changed Viewport._vp_input Viewport._vp_input_text Viewport._vp_unhandled_input ViewportContainer._input ViewportContainer._unhandled_input VisibilityEnabler._node_removed VisibilityEnabler2D._node_removed VisualInstance._get_visual_instance_rid VisualScript._get_data VisualScript._node_ports_changed VisualScript._set_data VisualScriptCustomNode._get_caption VisualScriptCustomNode._get_category VisualScriptCustomNode._get_input_value_port_count VisualScriptCustomNode._get_input_value_port_name VisualScriptCustomNode._get_input_value_port_type VisualScriptCustomNode._get_output_sequence_port_count VisualScriptCustomNode._get_output_sequence_port_text VisualScriptCustomNode._get_output_value_port_count VisualScriptCustomNode._get_output_value_port_name VisualScriptCustomNode._get_output_value_port_type VisualScriptCustomNode._get_text VisualScriptCustomNode._get_working_memory_size VisualScriptCustomNode._has_input_sequence_port VisualScriptCustomNode._script_changed VisualScriptCustomNode._step VisualScriptDeconstruct._get_elem_cache VisualScriptDeconstruct._set_elem_cache VisualScriptFunctionCall._get_argument_cache VisualScriptFunctionCall._set_argument_cache VisualScriptFunctionState._signal_callback VisualScriptNode._get_default_input_values VisualScriptNode._set_default_input_values VisualScriptPropertyGet._get_type_cache VisualScriptPropertyGet._set_type_cache VisualScriptPropertySet._get_type_cache VisualScriptPropertySet._set_type_cache VisualScriptSubCall._subcall VisualShader._input_type_changed VisualShader._queue_update VisualShader._update_shader VisualShaderNodeCustom._get_category VisualShaderNodeCustom._get_code VisualShaderNodeCustom._get_description VisualShaderNodeCustom._get_global_code VisualShaderNodeCustom._get_input_port_count VisualShaderNodeCustom._get_input_port_name VisualShaderNodeCustom._get_input_port_type VisualShaderNodeCustom._get_name VisualShaderNodeCustom._get_output_port_count VisualShaderNodeCustom._get_output_port_name VisualShaderNodeCustom._get_output_port_type VisualShaderNodeCustom._get_return_icon_type VisualShaderNodeCustom._get_subcategory WindowDialog._closed WindowDialog._gui_input ```
aki-cat commented 3 years ago

This is really out there, but I usually like to be explicit when writing code. Since the tag system is already implemented in 4.0, wouldn't it be useful to use it for overrides?

Like so:

@override
func _ready() -> void:
    # do stuff
    pass

# and so on for every "magic" method

It doesn't have to be this syntax exactly, but the point I'm trying to make here is that it's better to be explicit when you're overriding a method rather than silently shadow it. If you don't explicitly do it, it could show an error or warning.

Honestly I don't like "magic" names that by convention are supposed to be shadowed. I always prefer to be explicit so anyone can read the code know what it's doing. Also, it's useful to use underscore names as a way to signify you shouldn't call that method explicitly from outside that class (private member signifier by naming convention); in such cases like the given example accidental shadowing can occur when using this convention.

Xrayez commented 3 years ago

in such cases like the given example accidental shadowing can occur when using this convention.

I think we'd get the same problem either way, explicit or not, because there's just no core mechanism that determines whether those methods are internal or not. Currently, these internal callbacks can be overridden just like public methods, because they are technically public methods as well, even though they are not visible in documentation, autocompletion etc.

I'd even go as far to say that it's not a GDScript problem, it's core ClassDB problem, which indeed does not have an explicit way to mark methods as internal (so GDScript could properly implement it).

One of the reasons (or the only reason) why they're "exposed" is because some of these methods must be called deferred (even when used internally), and that requires registering those methods in ClassDB that start with underscore (yeah, that's how core distinguishes between internal and public methods, unless they are marked as virtual explicitly, like _ready). That's how I came up with godotengine/godot#42968.

fire commented 2 years ago

My argument is that godot uses _ to mean public access but an internal function. If we decide to implement this proposal, I should suggest renaming the current uses of _. We currently don't have a pattern for private functions that are accessible by scripting and this proposal is indirectly arguing for a private script method.

My opinion is this conflicts with existing decision of "public access for internal functions" and so we should revisit the earlier decision and choose that one or this one. The preference is for the decision made earlier in time.

nathanfranke commented 2 years ago

See also https://github.com/godotengine/godot/issues/32585