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

Cache allowed types in EditorResourcePicker #84443

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 4, 2023

EditorResourcePicker would gather allowed types every time they were needed. Worst case scenario is when the base type is Resource (so everything is allowed) and you are dragging a resource over the picker:

godot.windows.editor.dev.x86_64_OAeBoksjAv.mp4

This PR ensures that the types are gathered only once per picker.

@YuriSizov
Copy link
Contributor

I'm seeing double #51213 🙃 And then Juan removed it in #71683. It didn't get any proper reviews as far as I can see, so I dunno if it was justified.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 4, 2023

The problem with the previous caching was that it was static and probably wasn't refreshing properly (based on #71683 comment).
The caching added in this PR is performed for every new picker, so it won't be missing types.

btw I noticed that types are fetched when the picker is created, which might cause potential slowdowns if many pickers are created at once. Might be worth looking into, the type list is only needed when trying to assign something.

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 4, 2023

btw I noticed that types are fetched when the picker is created, which might cause potential slowdowns if many pickers are created at once. Might be worth looking into, the type list is only needed when trying to assign something.

That's why the cache was originally added like that. So only the first picker has to do the hard work, and the rest, if they share the base type, can benefit from it. We had really bad inspector refresh slowdowns before we added it.


Edit: We also agreed with Akien that it was okay that you need an editor restart to pick up new classes, because we already had this limitation for other global class related stuff. It was a conscious choice to accept this limitation.

If what you describe was a problem, then this new solution only helps with new pickers, but existing pickers would still have outdated types cached when global classes change.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 4, 2023

existing pickers would still have outdated types cached when global classes change.

That's unlikely to happen. You'd have to change global classes without refreshing the inspector with resource pickers. And even if that happens, you just need to reselect the object.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 4, 2023

I opened #84446
This will remove inspector slowdowns when updating.

Comment on lines +851 to +852
_ensure_allowed_types();
HashSet<StringName> allowed_types = allowed_types_with_convert;
Copy link
Member

Choose a reason for hiding this comment

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

Should we force updating the allowed types even if already cached, when changing base_type?

Like _ensure_allowed_types(bool p_force) that would bypass the !allowed_types_without_convert.is_empty() early return.

I'm not too familiar with this code but wouldn't this alleviate the issue with possibly requiring an editor restart to take new allowed types into account?

Copy link
Member Author

Choose a reason for hiding this comment

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

Editor restart is not required, only inspector refresh. When global classes change, all resource pickers already in scene tree will have outdated information and need to be recreated. Inspector pickers are recreated with every object edit, not sure if there are some that would stay outdated (maybe in e.g. project settings).

Also base type is set once per picker, idk if there are exceptions.

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.

Makes sense to me overall.

@akien-mga akien-mga merged commit d9bf750 into godotengine:master Apr 30, 2024
15 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.

3 participants