Fix ResourceSaver saving default value of Resource

[OzelotVanilla]

This PR solves the issue mentioned in this previous PR #80897.
That PR seems to be inactive for a few while, and according to the discussion #80897 (review) done by @dalexeev and @RandomShaper, this PR is proposed. The original PR just needs a few changes to fix the issue #80894.

Since I am new to this community yet, I am still learning the convention (and what considered as common sense) here. Hope this PR is not offensive.
Happy to close (abort) this PR if the original authors continue on their work.

If this PR is considered valid, I want to ask that:

• Should I add the two paticipants as author (make me commiter) or co-autor in the commit ?
• Should I add close statement to the linked issues and that original PR and related PR mentioned ?

---

Added by @RandomShaper:
Fixes #80894.
Supersedes #80897.

[RandomShaper]

@OzelotVanilla You can use annotations similar to the ones I've added to the bottom of the PR description you wrote. When a PR is intended to replace another one which is either stale or an agreedly worse approach, it's common practice to use the Supersedes word. Regarding issue(s) the PR may fix, you can add Fixes or Closes statements.

[RandomShaper]

This makes a lot of sense to me. However, these areas always tricky. In any case, since we're still in dev release stage, I believe this is a good moment to give this a go.

[OzelotVanilla]

This makes a lot of sense to me. However, these areas always tricky. In any case, since we're still in dev release stage, I believe this is a good moment to give this a go.

@RandomShaper Got that. Thank you for the explanation 😀 ! Later I will search around to see if there is any similar issue. When that is done, I would send a new comment here.

By the way I found Supersedes does not auto close that PR. Is this desired ? Should I add close statement to that PR again ?

[akien-mga]

By the way I found Supersedes does not auto close that PR. Is this desired ? Should I add close statement to that PR again ?

That's fine like this, since closing a PR is usually a maintainer decision so we do it manually.

[OzelotVanilla]

@RandomShaper I think no further issues could be added to this project. This PR is able to be merged now. Thank you for you all for reviewing this PR.

By the way do I need to edit the commit message to add you (@RandomShaper and @dalexeev) as co-author ?

[Repiteo]

Thanks!

[RandomShaper]

By the way do I need to edit the commit message to add you (@RandomShaper and @dalexeev) as co-author ?

As far as I can tell, we have no formal rules for that. However, I can confidently say that in a case like this no one would expect you to do so. You'd only add co-author annotations in cases were others' have made significant contributions to the code committed, especially if those are already in the form of code. An example of that is when you squash some commits of yours with sone from other people.

[CrabNickolson]

@akien-mga @OzelotVanilla I encountered an issue after this was merged. Resources inheriting from a C# script are now saved wrong. It seems PropertyUtils::get_property_default_value returns null for the script field, causing that field to never be saved.
This only happens for C# scripts, GD seems fine.
edit: this is wrong, see my comment and the bug report below

[OzelotVanilla]

@akien-mga @OzelotVanilla I encountered an issue after this was merged. Resources inheriting from a C# script are now saved wrong. It seems PropertyUtils::get_property_default_value returns null for the script field, causing that field to never be saved. This only happens for C# scripts, GD seems fine.

Thank you for reporting this ! I did not realise that and I should also give C# script a test.
(But since my PR is also following other's suggestion, I might need to ask in the develop channel on how to fix it.)

---

Edit: Do you think the root cause could be here:

godot/scene/property_utils.cpp

Lines 92 to 100 in 50c4b36

	// Handle special case "script" property, where the default value is either null or the custom type script.
	// Do this only if there's no states stack cache to trace for default values.
	if (!p_states_stack_cache && p_property == CoreStringName(script) && p_object->has_meta(SceneStringName(_custom_type_script))) {
	Ref<Script> ct_scr = get_custom_type_script(p_object);
	if (r_is_valid) {
	*r_is_valid = true;
	}
	return ct_scr;
	}

[CrabNickolson]

Actually, ignore what I said about the issue being that PropertyUtils::get_property_default_value returns null. I accidentally looked at the wrong resource while debugging the code.
The opposite is actually the case! PropertyUtils::get_property_default_value returns the script class, and the old function ClassDB::class_get_default_property_value returns null. And because the new function returns the C# script, it causes the equality check to pass and then skips over saving the property.

I'm not familiar enough with Godots codebase to really say how this fix this. You could just add a special case for the script property, but there is probably a better way.

[akien-mga]

@CrabNickolson Could you open an issue so we can track this regression properly as a blocker?

[CrabNickolson]

@akien-mga done, #107243

(also it turns out that this does affect GDScript, as long as the resource is a global class)