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 editor crash when a file can't be instantiated in scene #89106

Closed
wants to merge 1 commit into from

Conversation

luevano
Copy link
Contributor

@luevano luevano commented Mar 3, 2024

Fixes #89093
Possibly introduced by #88829 as this added code to show more information when instantiating a node/resource into the scene. Unsupported files (such as .tres as shown in the issue) crash the editor due to an index out of bounds when trying to get the child of the preview_node (there are no children).

Following video shows the fix based on the MRP uploaded. I don't think this messes with the intended behavior. Let me know if I should check for more cases.

2024-03-03.02-05-42.mp4

@AThousandShips AThousandShips added this to the 3.6 milestone Mar 3, 2024
@akien-mga akien-mga modified the milestones: 3.6, 4.3 Mar 3, 2024
@akien-mga akien-mga requested review from Calinou and KoBeWi March 3, 2024 09:44
@KoBeWi
Copy link
Member

KoBeWi commented Mar 3, 2024

While the fix is correct for the crash, the real issue is that the code checks whether drop is valid based on extensions. And Texture2D valid extensions includes tres, which means can_instantiate will be true for any Resource, while it shouldn't.

A better fix would be changing the code to use ResourceLoader::get_resource_type() (or whatever we use to check for resource type) instead of relying on extensions. This can be done in addition to your fix though.

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.

What I said can be done in follow-up. The current change is fine as a safeguard.

@ryevdokimov
Copy link
Contributor

ryevdokimov commented Mar 3, 2024

Whoops, I should have considered this scenario when writing the PR that introduces this.

If the fix that KoBeWi suggests is applied I believe that will also resolved this other issue of non-instantiable resources creating a fake "Create Node" history.

2024-03-03.10-01-43.mp4

@luevano
Copy link
Contributor Author

luevano commented Mar 3, 2024

Whoops, I should have considered this scenario when writing the PR that introduces this.

If the fix that KoBeWi suggests is applied I believe that will also resolved this other issue of non-instantiable resources creating a fake "Create Node" history.
2024-03-03.10-01-43.mp4

This also happens when trying to instantiate a node in itself, the "Create Node" shows:

godot.windows.editor.x86_64_ExxAqtNfbH.mp4

@luevano
Copy link
Contributor Author

luevano commented Mar 4, 2024

Whoops, I should have considered this scenario when writing the PR that introduces this.

If the fix that KoBeWi suggests is applied I believe that will also resolved this other issue of non-instantiable resources creating a fake "Create Node" history.
2024-03-03.10-01-43.mp4

Actually, on this PR build, it doesn't show the "Create Node" like you mentioned, checked with 4.3-dev4 and it actually crashes, too. What build are you using?; only inception'ed scenes like my last comment show up when applying this fix

@ryevdokimov
Copy link
Contributor

Ah sorry, I was using a build before my commit since what I already had open was just following the logic of KoBeWi statement. I can see now that your build would fix this issue too, and how my comment could be confusing.

@luevano
Copy link
Contributor Author

luevano commented Mar 4, 2024

Ah sorry, I was using a build before my commit since what I already had open was just following the logic of KoBeWi statement. I can see now that your build would fix this issue too, and how my comment could be confusing.

Hey, no issues at all, just wanted to clarify. I've implemented KoBeWi suggestions in the follow up PR, which handles this issue better. I'll look up about the case I commented to also fix that.

@akien-mga
Copy link
Member

Superseded by #89126.

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.

Dragging any Resource not recognized by the editor over the viewport crashes Godot
5 participants