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

Ignore ERR_FILE_CANT_OPEN error when loading #90269

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Apr 5, 2024

Follow-up to #85159
Fixes #80324

I didn't try to find out why there is yet another error for missing files, but my guess is that different loaders just return inconsistent values.

@KoBeWi KoBeWi added this to the 4.3 milestone Apr 5, 2024
@KoBeWi KoBeWi requested a review from a team as a code owner April 5, 2024 15:51
@KoBeWi KoBeWi force-pushed the sir,_your_file_cannot_be_opened branch from d5a111e to f0cbd38 Compare April 5, 2024 15:51
@akien-mga akien-mga changed the title Ignore ERR_FILE_CANT_OPEN error when loading Ignore ERR_FILE_CANT_OPEN error when loading Apr 5, 2024
@akien-mga
Copy link
Member

I believe that ERR_FILE_CANT_OPEN is meant to convey that the file exists, but cannot be opened (e.g. no read permission, or it's indeed corrupted). So it would be worth tracking where this error is (wrongly?) returned.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 5, 2024

Still, if a dependency is corrupted, the scene should open as if it was missing. The only unrecoverable case should be when the scene itself can't be loaded.

@akien-mga
Copy link
Member

Should we consider printing unexpected error code without returning early, so loading a scene always recovers gracefully from a broken dep?

@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 6, 2024
@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 6, 2024

I tried looking for the source of invalid error, but the code is too tangled >_> It comes from some thread function.

Should we consider printing unexpected error code without returning early, so loading a scene always recovers gracefully from a broken dep?

Not sure what do you mean by that. The condition I modified makes the method not return. I could print the error if it's in the list of exceptions, but loading the scene already fills output with errors, so I think it's clearly communicated.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 6, 2024

Nevermind, I found the source:

if (r_error) {
*r_error = ERR_FILE_CANT_OPEN;
}

The loader either returns OK or ERR_FILE_CANT_OPEN. I think it could return the error from FileAccess operation. I found the same code in some other loaders. Should I change them?

@akien-mga
Copy link
Member

Not sure if we should change those, maybe. We need to harmonize our error reporting so that we use errors more purposely if we have checks that rely on specific error codes.

Either way, for the time being I think this PR makes it better.

@akien-mga akien-mga merged commit 97b5c14 into godotengine:master Apr 8, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the sir,_your_file_cannot_be_opened branch April 8, 2024 09:39
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 8, 2024
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.

Scene file became corrupted/invalid.
2 participants