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

Make unstore AnimationLibrary if AnimationTree is assigned AnimationPlayer #85575

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Nov 30, 2023

Reported a problem regarding when to enable EditableChildren from @Mickeon.

AnimationTree should not store libraries in .tres when it has an AnimationPlayer, as the AnimationLibrary resource may be stored duplicately.

Since this fix concerns of data lost, I would appreciate if someone could please carefully test this fix.

Bugsquad edit:

@Mickeon
Copy link
Contributor

Mickeon commented Dec 1, 2023

It has been mentioned in RocketChat that this issue stems from the fact that you cannot reference sub-resources from another scene (at least not yet). While the solution here looks about right, it may be worth adding a comment on why this is done because it does look a bit out of place.

@fire
Copy link
Member

fire commented Dec 1, 2023

@SaracenOne You worked with referencing sub-resources from another scene too, want to take a look?

@akien-mga
Copy link
Member

Worth noting that this is pretty much reviving #82750. I do think that's a much safer ad hoc fix suitable for 4.2.x than #84168 (which might be the better/more thorough fix, but seems risky for a cherrypick).

Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

Yeah, upon revisting this, I think this approach for fixing this one specific bug is still relevant, though we will still need another PR to fix other resource duplication bugs, even mine or an alternative fix.

@SaracenOne
Copy link
Member

SaracenOne commented Dec 15, 2023

Since this is revisting a previous PR, I think this is okay to merge. That being said, the other PR is still indeed very much relevant since it fixes other crucial things that this PR does not address. I agree with @akien-mga my other PR is risky, but so far I've not been able to come up with an alternative approach for fixing the broader bug, which I'm hoping more people could review it. I know @lyuma had some thoughts on the potential implications of preserving foreign resource references, so I might see if I can chat to him about it.

@YuriSizov YuriSizov merged commit 3fef891 into godotengine:master Dec 16, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

@TokageItLab TokageItLab deleted the Make-unstore-library branch February 14, 2024 05:32
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