Never duplicate script with Resource.duplicate()

[Mickeon]

Fix #58031.

Excludes the script property of a Resource from being ever duplicated. This doesn't mean that a Script resource cannot ever be duplicated, however.

[Mickeon]

This PR would fix the same identical bug by adding a flag. #41923

However, it's 2 years old and the flags are reaching their limit. It's worth considering.

[asmaloney]

Thanks @Mickeon. I tested the changes and they fix the perplexing Parse Error: Class "Foo" hides a global script class. errors I was getting with my project.

[kleonc]

Might be worth adding a note in the docs that the script is not being duplicated even if subresources param is true.

BTW this note:

godot/doc/classes/Resource.xml

Line 26 in 61021c0

[b]Note:[/b] If [param subresources] is [code]true[/code], this method will only perform a shallow copy. Nested resources within subresources will not be duplicated and will still be shared.

doesn't seem true to me, it looks like this part would handle resursively duplicating subresources:

godot/core/io/resource.cpp

Lines 265 to 269 in 61021c0

	} else if (p.get_type() == Variant::OBJECT && (p_subresources || (E.usage & PROPERTY_USAGE_DO_NOT_SHARE_ON_DUPLICATE))) {
	Ref<Resource> sr = p;
	if (sr.is_valid()) {
	r->set(E.name, sr->duplicate(p_subresources));
	}

And indeed seems like they are being duplicated recusively, simple example:

class_name Res
extends Resource

@export var subresource: Resource = null

@tool
extends EditorScript

func _run():
	var r := Res.new()
	r.subresource = Res.new()
	r.subresource.subresource = Res.new()
	
	print_res_info(r)
	print_res_info(r.duplicate(false))
	print_res_info(r.duplicate(true))

func print_res_info(r) -> void:
	print("r=%s r.subresource=%s r.subresource.subresource=%s" % [r, r.subresource, r.subresource.subresource])

Output:

• v4.0.beta2.official [f8745f2] (so script is also duplicated):

r=<Resource#-9223370009428808011> r.subresource=<Resource#-9223370009412030794> r.subresource.subresource=<Resource#-9223370009395253577>
r=<Resource#-9223370009361699143> r.subresource=<Resource#-9223370009412030794> r.subresource.subresource=<Resource#-9223370009395253577>
  built-in:1 - Parse Error: Class "Res" hides a global script class.
  built-in:1 - Parse Error: Class "Res" hides a global script class.
  built-in:1 - Parse Error: Class "Res" hides a global script class.
r=<Resource#-9223370009328144684> r.subresource=<Resource#-9223370009277813039> r.subresource.subresource=<Resource#-9223370009227481395>

• this PR:

r=<Resource#-9223369792868520409> r.subresource=<Resource#-9223369792851726658> r.subresource.subresource=<Resource#-9223369792834949392>
r=<Resource#-9223369792801394962> r.subresource=<Resource#-9223369792851726658> r.subresource.subresource=<Resource#-9223369792834949392>
r=<Resource#-9223369792767840575> r.subresource=<Resource#-9223369792751063332> r.subresource.subresource=<Resource#-9223369792734308538>

• v3.5.1.stable.official [6fed1ff] (analogous example):

r=[Resource:103201] r.subresource=[Resource:103202] r.subresource.subresource=[Resource:103203]
r=[Resource:103204] r.subresource=[Resource:103202] r.subresource.subresource=[Resource:103203]
r=[Resource:103205] r.subresource=[Resource:103207] r.subresource.subresource=[Resource:103209]

So even in 3.x subresources are being duplicated recursively... 🤔 That note was added in #40210, not sure what were the exact reason (haven't read #30385 deeply) but it should be corrected too (might be done in a seperate PR though if you want to keep things simple here @Mickeon).

[kleonc]

And I'm not sure if not duplicating script is always desired, someone else would need to confirm/deny it. If there are situations where duplicating it is wanted then probably subresources parameter should be changed to some enum instead (like suggested in #30385 (comment)).

[Mickeon]

Might be worth adding a note in the docs that the script is not being duplicated even if subresources param is true.

Should do, can do.

So even in 3.x subresources are being duplicated recursively... 🤔 That note was added in #40210, not sure what were the exact reason (haven't read #30385 deeply) but it should be corrected too (might be done in a seperate PR though if you want to keep things simple here.

I could do it here, but I struggle to understand what is incorrect about it, at least at the time of writing this.

And I'm not sure if not duplicating script is always desired, someone else would need to confirm/deny it. If there are situations where duplicating it is wanted then probably subresources parameter should be changed to some enum instead

In those cases, wouldn't it be possible to write something like new_resource.set_script(r.get_script().duplicate) on the very next line of the var new_resource = r.duplicate()? Although, I suppose that by doing that, the original custom property values would be lost...

[Mickeon]

The entire description honestly smells, I think the behaviour should be re-analysed and the description rewritten. For now I'm just going to add another note to it in this PR.

[Mickeon]

@kleonc See #67072.

[akien-mga]

This makes sense, we discussed it in a PR meeting and couldn't find a reason why we'd want to duplicate Script resources.

We just need to decide if we prefer this approach or the one in #41923, which introduces PROPERTY_USAGE_ALWAYS_SHARE_ON_DUPLICATE as pendant to the existing PROPERTY_USAGE_DO_NOT_SHARE_ON_DUPLICATE.

[mhilbrunner]

PR review meeting: we prefer the implementation in #41923. Thanks for contributing!