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

Fix Shader and ShaderInclude resource loading #80705

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

bitsawer
Copy link
Member

@bitsawer bitsawer commented Aug 17, 2023

Possibly also helps with some other issues like #79324. There have been several reports of deleted or renamed Shader resources somehow "haunting" a project, reappearing or otherwise causing some strange issues. These are possibly caused by the Shader resource loading logic which never fails even if you try to load a completely nonexistent Shader or ShaderInclude file, see #73975 for more details.

This PR adds error checking when loading these resources. In the linked issue MRP the crash happens because the scene file contains invalid references to a deleted .gdshader file:

[ext_resource type="Shader" path="res://Shaders/planetShader.gdshader" id="1_3kkjf"]

and

shader = ExtResource("1_3kkjf")

Because currently loading this invalid resource actually succeeds it also makes the editor think that Shaders directory exists and when it tries to scan this invalid directory Godot crashes (because DirAccess:open() return value is not checked).

After this PR, trying to open the scene will correctly fail with an error message pointing to the missing file instead of silently creating invalid resources and then crashing Godot when trying to save the scene. If people have similar dangling resource references in their projects, they can no longer open and edit these scenes in the editor as the loading will fail. To fix this, they can either manually edit the .tscn file using an external text editor or create a temporary shader file where the missing one should be, then edit and adjust the scene inside the Godot editor. edit: Turns out there is also a scene dependency fixer functionality in the editor that can be used to fix this, live and learn.

This PR will also trigger some relatively untested code branches because previously Shader resource loading never failed. There are some conditions in the codebase that never failed previously, but after some testing they seem to work okay. Still, it's something to keep an eye out for.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 28, 2023
@akien-mga akien-mga merged commit 1c9e45f into godotengine:master Aug 28, 2023
@akien-mga
Copy link
Member

Thanks!

@bitsawer bitsawer deleted the add_shader_load_error_checks branch August 28, 2023 13:40
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

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.

Folder reappearing after being deleted and crashes editor when clicked
3 participants