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

Prevent Theme resource from emitting changes during bulk operations #49227

Merged
merged 1 commit into from
Jun 13, 2021

Conversation

YuriSizov
Copy link
Contributor

Bulk operations from #46593 and #46808 were progressively slow whenever executed with the Inspector open. This is due to each and every change to the edited Theme resource being propagated and causing a full Inspector rebuild. To circumvent this I've added an internal way to freeze change propagation in Theme and allowed the theme editor to use it directly.

This is not a very safe way to do this, though I have no other ideas currently. The fact that it's a private method only accessible due to being a friend class should be a warning enough to use with care, I hope. A short brainstorming discussion on RC didn't result in a better idea 🙃

The result is great, though, as the resource is updated very quickly now, only getting stuck on the last step as the editor itself is being updated. To make sure that the user understands what's going on, the theme import progress bar has been updated to hang on a very specific step:

godot windows tools 64_2021-05-31_17-26-24


This is potentially useful for all resources, or maybe even objects. Maybe this can be refactored at some point to something more universal.

@YuriSizov YuriSizov added this to the 4.0 milestone May 31, 2021
@YuriSizov YuriSizov requested a review from a team May 31, 2021 14:38
@KoBeWi
Copy link
Member

KoBeWi commented Jun 9, 2021

Do the changes happen all at once or is it stretched over multiple frames? If it's the former and the problem is inspector update, the issue could be fixed in the inspector by deferring the update, so that all changes are batched and inspector refreshes only once (similar to e.g. update() method in CanvasItems).

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Jun 9, 2021

Yes, the problem is that the inspector is being updated after each resource change, and it gets progressively slower when you, say, copy a default theme as it gets bigger and bigger. Not sure if and how deferring for the inspector can be implemented, but it would affect other things too, so may not be ideal.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 9, 2021

Not sure if and how deferring for the inspector can be implemented, but it would affect other things too, so may not be ideal.

Basically, if something triggers inspector update, don't update immediately, but wait one frame (deferred update) and set some dirty flag to true. If multiple refreshes happen on one frame, the inspector would update only once. I'd be surprised if it doesn't already work like that though.

@YuriSizov
Copy link
Contributor Author

If multiple refreshes happen on one frame, the inspector would update only once. I'd be surprised if it doesn't already work like that though.

Yes, I think it does work like that, but the problem is that copying the theme takes several seconds, which is why I've added a progress bar to begin with. And that takes several frames, hence multiple inspector updates.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 9, 2021

Well if it takes a few frames then it's correct fix. Something more universal isn't needed right now.

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.

As we discussed on chat, this is not ideal but we don't have any better solution in mind for now. It's still ad hoc enough so it should be fine :)

@akien-mga akien-mga merged commit 12e0f10 into godotengine:master Jun 13, 2021
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the editor-theme-freeze-changes branch June 13, 2021 15:51
YuriSizov added a commit to YuriSizov/godot that referenced this pull request Jun 20, 2021
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