Skip to content
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

Reimplement Resource._setup_local_to_scene & deprecate signal #67080

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Oct 8, 2022

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_requested signal.
This signal can only be useful when connecting it in the custom Resource's own _init. It can be called manually with setup_local_to_scene. However, whether or not setup_local_to_scene should 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.

@Mickeon Mickeon requested review from a team as code owners October 8, 2022 12:54
@Mickeon Mickeon force-pushed the resource-virtual-local-to-scene-setup branch from f8727b3 to e3fff79 Compare October 8, 2022 13:18
@Chaosus Chaosus added this to the 4.x milestone Oct 8, 2022
@Mickeon Mickeon force-pushed the resource-virtual-local-to-scene-setup branch 3 times, most recently from bf0aa2e to e9662ac Compare February 11, 2023 22:19
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 11, 2023

Rebased. Bumping this up.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 15, 2023

The signal should be deprecated (and still functional) instead of being removed.
Also it should be a proper virtual method with GDVIRTUAL_BIND/CALL.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 15, 2023

So, wait, is the implementation on the PR not quite right? How to go about it?

@KoBeWi
Copy link
Member

KoBeWi commented Aug 15, 2023

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 GDVIRTUAL_CALL() (AFAIK it's related to GDExtension).

Then again, I see some methods still called via get_script_instance(), so I'm not sure if it's legacy or there are cases where it should be used 🤔

@Mickeon Mickeon force-pushed the resource-virtual-local-to-scene-setup branch from e9662ac to 9c4861b Compare September 6, 2023 16:35
@Mickeon Mickeon force-pushed the resource-virtual-local-to-scene-setup branch from 9c4861b to 75b5f45 Compare September 6, 2023 21:33
@Mickeon Mickeon changed the title Reimplement Resource._local_to_scene_setup & remove signal Reimplement Resource._local_to_scene_setup & deprecate signal Sep 6, 2023
@Mickeon Mickeon force-pushed the resource-virtual-local-to-scene-setup branch from 75b5f45 to cab6e5c Compare September 7, 2023 13:36
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 7, 2023

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

@YuriSizov
Copy link
Contributor

renames it to _local_to_scene_setup to better suit other similar virtual methods

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 _ prefix.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 8, 2023

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 _ prefix.

I'm not sure why I made that decision last year. It's likely because _local_to_scene_setup sounded more natural. It's the method where the setup takes place, and...

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.
I could just bring back the old virtual method as it was without any conversion. I'm surprised no one mentioned it until the last second.

@YuriSizov
Copy link
Contributor

I'm surprised no one mentioned it until the last second.

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.

@Mickeon Mickeon force-pushed the resource-virtual-local-to-scene-setup branch from cab6e5c to 4915538 Compare September 8, 2023 12:05
@Mickeon Mickeon changed the title Reimplement Resource._local_to_scene_setup & deprecate signal Reimplement Resource._setup_local_to_scene & deprecate signal Sep 8, 2023
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 8, 2023

Update the PR to change the name back to 3.x's equivalent.

Modified the description of _setup_local_to_scene slightly because I wasn't quite happy with it.
And added a deprecated message underneath setup_local_to_scene and setup_local_to_scene_requested.

@YuriSizov YuriSizov self-requested a review September 8, 2023 12:14
doc/classes/Resource.xml Outdated Show resolved Hide resolved
doc/classes/Resource.xml Outdated Show resolved Hide resolved
doc/classes/Resource.xml Outdated Show resolved Hide resolved
<return type="void" />
<description>
Override this method to customise the newly duplicated resource created from [method PackedScene.instantiate], if the original's [member resource_local_to_scene] is set to [code]true[/code].
[b]Example:[/b] Set a random [code]damage[/code] value to every local resource from an instantiated scene, excluding the original.
Copy link
Member

@KoBeWi KoBeWi Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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 = 0

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Mickeon Mickeon Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Mickeon Mickeon force-pushed the resource-virtual-local-to-scene-setup branch from 4915538 to e85f2cb Compare September 9, 2023 09:57
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 9, 2023

The final stretch.

I'm leaving that conversation unsolved because it intrigues me.

@Mickeon Mickeon force-pushed the resource-virtual-local-to-scene-setup branch from e85f2cb to 4014c80 Compare September 9, 2023 11:24
Reimplements the virtual method _setup_local_to_scene, lost in godotengine#51970

Also deprecates the redundant `setup_local_to_scene_requested` signal.
@Mickeon Mickeon force-pushed the resource-virtual-local-to-scene-setup branch from 4014c80 to 79ce0c6 Compare September 9, 2023 11:51
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Sep 14, 2023
@akien-mga akien-mga merged commit 19057c0 into godotengine:master Sep 27, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the resource-virtual-local-to-scene-setup branch December 30, 2023 11:43
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