Skip to content

Enhance bindings of deep resource duplication #107770

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

Merged
merged 1 commit into from
Jun 21, 2025

Conversation

RandomShaper
Copy link
Member

Two changes:

  1. Variant.duplicate_deep() argument is now told to be of the type enum::Resource.DeepDuplicateMode, instead of the wrong seemingly global enum::Resource.DeepDuplicateMode. See the excerpt below.
  2. A few renames are made in the bindings, to remove the redundant 'resource' word. This is why I marked this PR as compat breaking. However, let's bear in mind this is compat breakage compared to prev 4.5 releases. Renames are these:
    • Resource.ResourceDeepDuplicateMode -> Resource.DeepDuplicateMode
    • Resource.RESOURCE_DEEP_DUPLICATE_* -> Resource.DEEP_DUPLICATE_*

Now, Variang::deep_duplicate() looks like this in the API JSON:

{
	"name": "duplicate_deep",
	// [...]
	"arguments": [
		{
			"name": "deep_subresources_mode",
			"type": "enum::Resource.DeepDuplicateMode", // <-- This is the improvement.
			"default_value": "1"
		}
	]
}

Fixes #107138.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Makes sense to me

ClassDB::bind_integer_constant(get_class_static(), StringName("ResourceDeepDuplicateMode"), "RESOURCE_DEEP_DUPLICATE_NONE", RESOURCE_DEEP_DUPLICATE_NONE);
ClassDB::bind_integer_constant(get_class_static(), StringName("ResourceDeepDuplicateMode"), "RESOURCE_DEEP_DUPLICATE_INTERNAL", RESOURCE_DEEP_DUPLICATE_INTERNAL);
ClassDB::bind_integer_constant(get_class_static(), StringName("ResourceDeepDuplicateMode"), "RESOURCE_DEEP_DUPLICATE_ALL", RESOURCE_DEEP_DUPLICATE_ALL);
// Therefore, we can't use BIND_ENUM_CONSTANT here because it would
Copy link
Member

Choose a reason for hiding this comment

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

Missing the end of this sentence.

Wouldn't it make sense to just duplicate (heh) this enum in Resource so it can be exposed normally, and just cast it to the Variant one where relevant?

Copy link
Member Author

@RandomShaper RandomShaper Jun 20, 2025

Choose a reason for hiding this comment

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

I guess so. On the one hand, this case is already very special (the enum is global in C++ but scoped to Resource for scripting), so a little bit of special binding code doesn't seem out of place and makes the "bridging" very explicit. Not having to repeat the enum with the RESOURCE_ prefixes removed is an upside as well. On the other hand, what you suggest is more similar to what is already happening with certain enums in core_bind.h. If you ask me, I'd take the opposite direction, that is, integrating those duplicates.

I don't really have a strong opinion beyond personal taste.

In the meantime, I've fixed the sentence.

@RandomShaper RandomShaper force-pushed the fix_res_dupe_bindings branch from f3aeaf8 to 7dc37bd Compare June 20, 2025 16:40
@akien-mga akien-mga merged commit a0f9f5d into godotengine:master Jun 21, 2025
39 of 40 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

ResourceDeepDuplicateMode enum is wrongly formatted as a global enum in extension_api.json in method definitions.
3 participants