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

On bad array access error, bubble up the error #99914

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alecmeyer
Copy link

Addresses issue #98519 "Importing a large 400mb gltf/glb file crashes at 99%"

I would like to correct something incorrect that I said in a previous comment. I erroneously said before that upon importing the corrupt glb file to a new Godot project for the first time, it would remain unresponsive indefinitely. This was not actually the case. In truth, Godot only remains responsive for a very long time (about 10 minutes on my machine) but it will eventually arrive at the bad unsigned integer square brackets operation that I described, whether on the first try or any succeeding tries.

My fix does resolve the crashing for this case. It eventually returns Godot to a responsive state (as opposed to being unresponsive indefinitely), and communicates the error via notifications and on the console. It notably does NOT revert the state of the project to prior to the corrupt import.

After the exception is caught:
image
image
image

I believe the red X icon in the FileSystem correctly indicates that the file is corrupt, which is nice.
Additionally, I am under the impression that throwing an exception from the STD library is unconventional in the codebase but I am not familiar with the desirable standard.

I would be very pleased to make whatever modifications are seen fit.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Hi there! Thanks for your PR.

Unfortunately, for the reasons I stated, you will need to change your code to handle errors in a Godot-way that is not relying on a try/catch.

(You can search the codebase, there is a lot of error handling by passing Error *r_error as a parameter if the function called returns something already)

scene/resources/3d/importer_mesh.cpp Outdated Show resolved Hide resolved
editor/import/3d/resource_importer_scene.cpp Outdated Show resolved Hide resolved
@alecmeyer
Copy link
Author

alecmeyer commented Dec 4, 2024

I have modified the pull request to longer use the STL. Instead I have tried to imitate the style of using the error macros that I have found in the codebase. Doing so involved changing the parameters of many functions in order to pass down or bubble up error objects in order to read them in the desirable scope and act accordingly.

image_2024-12-04_01-13-31
I want to note that upon importing the large glb to a new project for the first time, Godot may remain unresponsive for ~8 minutes with no visual indication that it is still working.

The PR still fixes the bug as intended. After a long time, Godot becomes responsive again. Error notifications are displayed in the editor and in the console. As before, some files that were part of the import do appear in the FileSystem despite the failed import. The glb file appears to have been marked corrupted or failed.
image_2024-12-04_02-17-16
image_2024-12-04_02-17-26
image_2024-12-04_02-17-40

@fire fire changed the title Conditionally throw bad array access exception, bubble up, and catch On bad array access error, bubble up error Dec 4, 2024
@fire fire changed the title On bad array access error, bubble up error On bad array access error, bubble up the error Dec 4, 2024
@alecmeyer
Copy link
Author

Sorry for requesting review again before passing tests. I thought I had to request review in order to see test results.

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.

3 participants