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

Allow opening scenes with missing scene dependency #86781

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jan 4, 2024

When a scene has instances of scenes that no longer exist in the project, you won't be able to open them, with the well-known "Scene invalid/corrupt" error. This PR fixes that.

QdRyle2wEe.mp4

However, while it technically works (i.e. you can open the scene), it could use some improvements. I wanted it to at least display path of the missing scene, but I wasn't sure how to obtain it (in the video it shows as TODO). Also, contrary to what the tooltip says, saving the scene will result in data loss. The instance will be gone and any editable children data is lost. It would be nice to preserve at least the instance; if it's not possible/too difficult, changing the tooltip would be fine for now too.

@Mickeon
Copy link
Contributor

Mickeon commented Jan 4, 2024

Would be nice to preserve at least the instance

It would be nice if InstancePlaceholder were capable of doing that.

@Zireael07
Copy link
Contributor

@Mickeon Something for a followup PR I think?

@Mickeon
Copy link
Contributor

Mickeon commented Jan 4, 2024

Yeah. For the focus of this PR, changing the tooltip to not blatantly lie may be good enough.

@Mickeon
Copy link
Contributor

Mickeon commented Jan 4, 2024

Perhaps MissingNode could have a Dictionary containing NodePaths pointing to the lost properties and values. When loading a PackedScene in the editor, it is detected and rebuilt if possible, or left as is. It sounds extremely hacky but... Yeah.

@KoBeWi KoBeWi force-pushed the scenes_that_are_no_longer_with_us branch from a716162 to 7a9f940 Compare January 4, 2024 21:39
@KoBeWi KoBeWi marked this pull request as ready for review January 4, 2024 21:39
@KoBeWi KoBeWi requested a review from a team as a code owner January 4, 2024 21:39
@KoBeWi KoBeWi force-pushed the scenes_that_are_no_longer_with_us branch from 7a9f940 to 39a8d77 Compare January 4, 2024 22:39
@KoBeWi KoBeWi requested a review from a team as a code owner January 4, 2024 22:39
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 4, 2024

I managed to make the path available using a questionable hack 😬 Though for some reason it's not always reliable (it seems to depend on some cache).
I also adjusted the message:
image
If the path can't be determined for some reason, there is a slightly different massage that node is unknown. For now the instance is completely discarded.

In total there are 3 paths where loading scene can fail: instances, placeholders and inheritance. The former 2 are handled by this PR, the last one I think is not recoverable.

@reduz
Copy link
Member

reduz commented Feb 13, 2024

I think this looks good but keep in mind sometimes dependencies fail to open because they miss resources and they should still be openable anyway, so maybe this should only really be used in case the scene is not found.

@akien-mga
Copy link
Member

Will need a rebase.

@KoBeWi KoBeWi force-pushed the scenes_that_are_no_longer_with_us branch from 39a8d77 to 54d96cd Compare February 13, 2024 16:13
@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 13, 2024

Rebased. Seems like someone added a description for recording_properties in the meantime. I replaced it with mine, as it provides more information.

@reduz
Copy link
Member

reduz commented Feb 13, 2024

give me a second before merging this, I want to check something

@reduz
Copy link
Member

reduz commented Feb 13, 2024

Alright, I think its good to merge, but I think what we still need to have an option to recursively fix dependencies on subresources that don't load because of this.

@KoBeWi KoBeWi force-pushed the scenes_that_are_no_longer_with_us branch from 54d96cd to be4cbee Compare February 13, 2024 18:46
@akien-mga akien-mga merged commit 921b656 into godotengine:master Feb 13, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the scenes_that_are_no_longer_with_us branch February 13, 2024 23:06
@Flavelius
Copy link
Contributor

Flavelius commented Mar 4, 2024

I tried to address something like this a while ago aswell, and while it was more of a hackfix (#75103), i think not losing nodes is still the actual desire, as some sources might just have been renamed or moved; where it shouldn't be assumed they are actually missing.

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