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

Clarify that Resource.duplicate(true) doesn't duplicate subresources inside Array or Dictionary properties #94142

Merged

Conversation

AdriaandeJongh
Copy link
Contributor

Partially addresses #74918 by documenting it. The issue has been bugging a lot of people for at least a year now, and with no fix in sight the documentation might as well mention it to save a lot of sweat and tears.

@AdriaandeJongh AdriaandeJongh requested a review from a team as a code owner July 9, 2024 17:23
@AThousandShips AThousandShips added enhancement discussion documentation cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 9, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Jul 9, 2024
doc/classes/Resource.xml Outdated Show resolved Hide resolved
@AThousandShips AThousandShips requested review from a team July 9, 2024 17:59
@AThousandShips
Copy link
Member

I believe this is the way to go for this, even if we consider the current behavior a clear bug changing it without adding more options and complexity would IMO break compatibility with existing code. We can work further with improving the behavior (IMO by adding a flag for this), but I think the first step is to reduce confusion by adjusting the documentation to match reality

@Mickeon Mickeon self-requested a review July 10, 2024 09:06
@Frozenfire92
Copy link
Contributor

I think it would be worthwhile including a workaround as well like mentioned in this comment #74918 (comment)

@Mickeon
Copy link
Contributor

Mickeon commented Jul 11, 2024

The workaround is a bit too broad and non-exhaustive in my opinion. It has to be copied and pasted for every array/dictionary property. At the same time, anything more would be too verbose to fit the description.

@Frozenfire92
Copy link
Contributor

@Mickeon Fair enough, I think I might try and play around with an editor warning for when this issue happens 🤔

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.

The wording looks good to me

doc/classes/Resource.xml Outdated Show resolved Hide resolved
Comment on lines +45 to +46
- Subresource properties with the [constant PROPERTY_USAGE_ALWAYS_DUPLICATE] flag are always duplicated.
- Subresource properties with the [constant PROPERTY_USAGE_NEVER_DUPLICATE] flag are never duplicated.
Copy link
Member

Choose a reason for hiding this comment

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

That reminds me, these property flags are currently not surfaced in the inspector tooltips or class reference in any way. We should find a way to do this somehow.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jul 17, 2024
@akien-mga akien-mga removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 17, 2024
@akien-mga akien-mga merged commit 5257ba1 into godotengine:master Jul 17, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@AdriaandeJongh AdriaandeJongh deleted the resources-duplicate-doc-update branch July 20, 2024 19:16
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.

6 participants