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 unsupported Resource is dropped in scene #89126

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

luevano
Copy link
Contributor

@luevano luevano commented Mar 4, 2024

Follow up on #89106

Change CanvasItemEditorViewport::can_drop_data to check if files are valid Resource types (PackedScene or Texture2D) instead of checking for extension types (where .tres is valid for Texture2D but it's not an exclusive extension for it).

Change safeguard check from #89106 to an error message as suggested. Though, from my testing I can't seem to trigger this error message as the Resource type check handles this, too.

Changed nested if statements to exit immediately to avoid unnecessary code nesting.

Also fixes #89093, so it actually supersedes #89106

@akien-mga
Copy link
Member

CC @ryevdokimov

@luevano
Copy link
Contributor Author

luevano commented Mar 4, 2024

I also fixed an issue that when dropping a scene into itself (cyclic dependency) would create an undo-redo history action and display "Create Node" in the godot diagnostics window; I found it while testing this and it's shown here: #89106 (comment) (in my old PR that is now superseded by this PR). It now behaves like this:

godot.windows.editor.x86_64_0s6IjBqK0L.mp4

The only downside is that there is no easy way to display the "cyclic dependency" error that I know of (that doesn't pollute the logs). Can I include this change in this PR, too? It's related.

Copy link
Contributor

@ryevdokimov ryevdokimov 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.

2024-03-04.08-24-20.mp4

@luevano
Copy link
Contributor Author

luevano commented Mar 4, 2024

Any comment on my last message @ryevdokimov / @KoBeWi ? I haven't included the changes to avoid dropping cyclic dependencies as I found a fix after the fact

@KoBeWi
Copy link
Member

KoBeWi commented Mar 4, 2024

I think that fix should go to separate PR, unless it's closely related.

@luevano
Copy link
Contributor Author

luevano commented Mar 4, 2024

It's a change on the same can_drop_data and minor one in the _cyclical_dependency_exists to mark it as const.

@luevano luevano force-pushed the fix-89093-followup branch from 5f2abd9 to c0467aa Compare March 4, 2024 16:07
@KoBeWi
Copy link
Member

KoBeWi commented Mar 4, 2024

I think that other change should be in _perform_drop_data(), to prevent creating undo actions if no node was created. Thus it's better as a separate fix.

@luevano
Copy link
Contributor Author

luevano commented Mar 4, 2024

Edit: sorry, didn't see your latest comment. I did try doing it there, but I failed. Will look into it with more time later.

To keep this simple and prioritize the crash fix, I'll leave it as is. Once this is merged I can create a PR for the change I'm describing

@akien-mga akien-mga merged commit c4a46e0 into godotengine:master Mar 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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