Reimplement Resource._setup_local_to_scene & deprecate signal#67080
Conversation
f8727b3 to
e3fff79
Compare
bf0aa2e to
e9662ac
Compare
|
Rebased. Bumping this up. |
|
The signal should be deprecated (and still functional) instead of being removed. |
|
So, wait, is the implementation on the PR not quite right? How to go about it? |
|
This part is wrong: if (get_script_instance()) {
Callable::CallError ce;
get_script_instance()->callp(SNAME("_local_to_scene_setup"), nullptr, 0, ce);
}All virtual calls should now use Then again, I see some methods still called via |
e9662ac to
9c4861b
Compare
9c4861b to
75b5f45
Compare
_local_to_scene_setup & remove signal_local_to_scene_setup & deprecate signal
75b5f45 to
cab6e5c
Compare
|
This PR has been updated to to work with GDExtension, because the cyclic dependency has been thankfully solved. It also is no longer compatibility-breaking, because the signal is deprecated, but functionality is still retained (to my dismay). |
Can you give an example? Typically a virtual method that needs to be implemented to change the behavior of a non-virtual methods is called exactly the same with the |
I'm not sure why I made that decision last year. It's likely because In hindsight I can't explain a good reason to keep it. Perhaps this is because virtual method names may have been standardized more across the board. |
Well, we got to reviewing this PR only a few weeks back :P And it's more of a codestyle pass thing, while previous comments were about the implementation. |
cab6e5c to
4915538
Compare
_local_to_scene_setup & deprecate signal_setup_local_to_scene & deprecate signal
|
Update the PR to change the name back to 3.x's equivalent. Modified the description of |
doc/classes/Resource.xml
Outdated
There was a problem hiding this comment.
What "excluding the original" refers to? I did a test with this method in a tool script and every Resource instance has this called.
EDIT:
I used this for testing:
LocalTest.zip
There was a problem hiding this comment.
"Excluding the original" refers to the resource every local resource has been duplicated from. The method is called on every new one, but the original Resource, basically the following in your scene, is not affected by this:
[sub_resource type="Resource" id="Resource_gw2eg"]
resource_local_to_scene = true
script = ExtResource("1_khad7")
damage = 0You may actually change this Resource at run time without affecting the duplicates. However, every newly instantiated scene will create its own local resources with the original properties in mind.
This is worth knowing especially for Resources like Stylebox, or resources saved to disk.
Under the hood you may If you also feel like this is necessary information and/or have better ways to phrase it, feel free to suggest.
There was a problem hiding this comment.
But if I open HolderOfLocal.tscn and save it, it will be modified. If I run the scene and print the value, it will also have a different value.
@export var res: Resource
func _ready() -> void:
print(res.damage)So when this method doesn't get called?
There was a problem hiding this comment.
That is... unexpected. Oh dear.
For now I'll remove the "except" stuff, but that feels like should not happen. Maybe this bug, if it is one, only happens when a Resource is contained within a PackedScene?
4915538 to
e85f2cb
Compare
|
The final stretch. I'm leaving that conversation unsolved because it intrigues me. |
e85f2cb to
4014c80
Compare
Reimplements the virtual method _setup_local_to_scene, lost in godotengine#51970 Also deprecates the redundant `setup_local_to_scene_requested` signal.
4014c80 to
79ce0c6
Compare
|
Thanks! |
Requires #81388.
This PR reimplements the virtual method
Resource._setup_local_to_scene, that has been lost since #51970. It also gives it a much, much better description compared to 3.x:Finally, deprecates the now redundant
setup_local_to_scene_requestedsignal.This signal can only be useful when connecting it in the custom Resource's own
_init. It can be called manually withsetup_local_to_scene. However, whether or notsetup_local_to_sceneshould be exposed to the user in the first place is very up to debate to me (see my struggles attempting to update the description in #67072 and #67082.I wish I could just remove it, to be honest. Only like, one or two people in the universe may be using it.