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

Enable QuickOpen to see scripted resources #62417

Merged
merged 1 commit into from
Sep 18, 2022

Conversation

willnationsdev
Copy link
Contributor

@willnationsdev willnationsdev commented Jun 26, 2022

Updates EditorQuickOpen so that resources with global script classes attached to them are properly recognized and handled.

Related Issues:

Related PRs:

Sibling PRs:

Considerations:

  • None.

Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

This looks good to me, again I think we need to clean up the EditorData usage for these things, but it can be moved as a 4.1 task at this point.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 16, 2022

So my question is, why there are so many core changes required for this when #62413 improves the resource picker with relatively few inner adjustments? #62413 makes drag'n'drop from the filesystem dock work for all resources, which is about the same amount of checking as EditorQuickOpen needs. So why is all this needed?

Edit: Ah, I see. It depends on #60606 changes for some things. But I don't believe this PR actively needs anything from it more than #62413 does, right? I think we should strip this one to be more like #62413, then we can consider and review #60606 and its more fundamental changes.

After that both EditorQuickOpen and EditorResourcePicker can be updated to use the filesystem cache. Just leave a TODO note like you did for the resource picker for now.

Edit 2: Regarding your OG note:

Without this, teaching EditorQuickOpen to display applicable resource files would be prohibitively expensive since every file with the same native base type would need to be fully loaded just to check 1) if it has a script attached, 2) whether the script is a global class, and 3) whether the script class matches the exported property's type constraints.

How slow does it get in practice? We already have similar issue in other places of the UI, like the Create Node/Resource dialog. But I guess here it is magnified by the number of resources compared to the number of scripts defining classes, which would be significantly larger. Worst case, we can implement this and disable until we fix the performance with cache.

Because we'd love to merge this, but the core stuff seems problematic and in need of way more polish. We assume your own part is significantly smaller and clearer here.

@willnationsdev
Copy link
Contributor Author

@YuriSizov I went ahead and added a bool property to toggle it for now, and it just goes ahead and loads the resources. If it ends up being too much of a performance issue, then we can just turn it off, as you mentioned, until a proper solution to caching can be implemented.

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 👍

@akien-mga akien-mga merged commit 7a0a3fe into godotengine:master Sep 18, 2022
@akien-mga
Copy link
Member

Thanks!

@KoBeWi
Copy link
Member

KoBeWi commented Sep 18, 2022

What did this PR actually change? I could see custom resources in quick open before that and the custom icons are still not displayed.

@willnationsdev
Copy link
Contributor Author

@KoBeWi Oh, huh. I didn't really quite get the chance to test it again after I stripped out the base stuff that Atlinx had contributed. It was supposed to ensure that resources with scripted type names actually show up in the resource list inside the QuickOpen dialogue, and that their custom icon would be displayed. Not sure why that wouldn't have worked since it was working earlier, and the only thing Atlinx's code should have contributed would've been caching the class info in the EditorFileSystem and me retrieving the data from there (which was changed to me just loading the resource directly).

@KoBeWi
Copy link
Member

KoBeWi commented Sep 18, 2022

You mean the quick open that shows when you press Shift+Alt+O (default)? Custom resources have always appeared in there. It works even in 3.5.

But yeah, the icons don't show up. And this PR didn't fix that xd

@willnationsdev
Copy link
Contributor Author

@KoBeWi No, that's something else entirely. If you export a scripted resource type, and then attempt to use the quickload option in the Inspector dropdown to populate the exported property, (where the QuickOpen dialogue is configured to have a base type associated with the user-defined class name), then the list of displayed resources would not previously respect that constraint. That is what was fixed by this PR. I wasn't aware of other issues because that's the only use case I was really experimenting with when I was implementing the other features. I can't recall actually setting up the icon bit though in my test cases, so never really confirmed that the icon was showing up. In short, if you fix the other bug that makes the icon not show up, then the fixed version will properly display the base global script class's icon if the QuickOpen dialogue is configured to use that global script class (which it wouldn't have done before).

@KoBeWi
Copy link
Member

KoBeWi commented Sep 19, 2022

Makes sense. Though I tested it and I get empty dialog for some reason.

godot.windows.tools.x86_64_p2VkPKKmzE.mp4

It should show the TestTest.tres file, no?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 19, 2022

btw

If it ends up being too much of a performance issue, then we can just turn it off, as you mentioned, until a proper solution to caching can be implemented.

Yep, this should be disabled for now. Here's me trying to "quick" open a script:

godot_ohUkBeJcjW.mp4

akien-mga added a commit to akien-mga/godot that referenced this pull request Sep 21, 2022
As expected while reviewing godotengine#62417 this is indeed not practical
without a better system to retrieve this information.

Fixes godotengine#66179.
reduz added a commit to reduz/godot that referenced this pull request Jan 21, 2023
* Works for binary and text files.
* Makes EditorQuickOpen work with custom resources again.
* Information is cached and easily accessible.

Properly fixes godotengine#66179. Supersedes godotengine#66215 and supersedes godotengine#62417

**WARNING**: This required breaking backwards binary compatibility (.res and .scn files). Files saved after this PR is merged will no longer open in any earlier versions of Godot.
Streq pushed a commit to Streq/godot that referenced this pull request Feb 9, 2023
* Works for binary and text files.
* Makes EditorQuickOpen work with custom resources again.
* Information is cached and easily accessible.

Properly fixes godotengine#66179. Supersedes godotengine#66215 and supersedes godotengine#62417

**WARNING**: This required breaking backwards binary compatibility (.res and .scn files). Files saved after this PR is merged will no longer open in any earlier versions of Godot.
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.

6 participants