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

Simplify Subresource Saving #62318

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented Jun 22, 2022

Redo edited subresource (and resource) saving in a much more simplified way. It should be much faster too and help considerably improve save times on large scenes.

I think this should work (unless I am missing something) and be faster than what is there.
Fixes #55885 and supersedes #57077.

I am not 100% entirely convinced that this approach works. Everything points to the fact it should and it seems to work for me, but just in case make sure to test well!

@TokageItLab
Copy link
Member

Is it related to #62287?

Copy link
Contributor

@Maran23 Maran23 left a comment

Choose a reason for hiding this comment

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

I just tested it and this indeed fixes the bug and works well.

This implementation is quite smart, I remember while debugging that I also found out that ResourceCache::get_cached_resources(&cached); will also contain all sub resources but I didn't know how to proceed from there.

editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Jun 22, 2022

Seems like this PR causes changes to unopened scenes. When you have an instanced scene, but it's not opened in the editor, saving the parent scene will still save all instanced scenes.

It should be much faster too and help considerably improve save times on large scenes.

So I guess it's the opposite 😅

@reduz
Copy link
Member Author

reduz commented Jun 23, 2022

@KoBeWi Strangely I don't see why that would be different than what it was before, but in either case I don't think applying this to PackedScenes makes any sense, so I could manually skip them?

@reduz reduz force-pushed the simplify-subresource-saving branch from 7952338 to 925e11c Compare June 23, 2022 13:06
editor/editor_node.cpp Outdated Show resolved Hide resolved
Redo edited subresource (and resource) saving in a much more simplified way.
I think this should work (unless I am missing something) and be faster than what is there.
It should also supersede godotengine#55885.

I am not 100% entirely convinced that this approach works, but I think it should so please test.
@reduz reduz force-pushed the simplify-subresource-saving branch from 925e11c to 9eb5f2a Compare June 23, 2022 19:25
@reduz
Copy link
Member Author

reduz commented Jun 23, 2022

@KoBeWi Please test again to see if you encounter the same behavior

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Doesn't cause (obvious) problems anymore and fixes the issue.
Can't say much about the code though, other than it probably makes sense.

@akien-mga akien-mga merged commit 09c5849 into godotengine:master Jun 27, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 27, 2022
akien-mga added a commit that referenced this pull request Jun 27, 2022
@akien-mga
Copy link
Member

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
8 participants