Forbid access to internal, private, non-virtual C++ methods and callbacks via script #2285
Labels
Comments
Xrayez
changed the title
|
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 I understand that it is risky to rely on such methods, but often this is the only way to solve a certain problem. So I am for careful manual selecting of methods that should be hidden and those that could be exposed. |
|
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. |
|
Simple script shows that there is 671 methods which starts with List of all functions |
|
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" methodIt 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. |
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 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 |
|
My argument is that godot uses 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. |
|
See also godotengine/godot#32585 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
BakedLightmapData.new()._set_user_data([])crashes Godot godot#45996NoiseTexture.new()._thread_done(BoxShape)crashes Godot godot#46000ProximityGroup.new()._proximity_group_broadcast(String(),false)crashes Godot godot#46002RigidBody.new()._direct_state_changed(BoxShape.new())crashes Godot godot#46003VisualShader.new()._input_type_changed(10,10)crashes Godot godot#46011TileMap.new()._set_tile_data([...])causes invalid read godot#46015Control.new()._edit_set_state({})crashes Godot godot#46017Control.new()._edit_set_position(Vector2(1000,1000))crashes Godot godot#46018ProceduralSky.new()._thread_done(Image.new())crashes Godot godot#46025you'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()inNode2Dto schedule world update:As it turns out,
_update_callbackuses a similar mechanism to schedule draw calls toupdate(), but since I'm overriding it, nothing is actually drawn, and I couldn't figure out why until I renamed_update_callbacktoupdate_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.
The text was updated successfully, but these errors were encountered: