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 noticeable freeze after saving a scene #93147

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

Hilderin
Copy link
Contributor

Fixes #93104 and maybe #92515

After testing with a project having a lot of Resources I was able to reproduce the problem.

MRP:
test-godot-resource-icon.zip

The problem came from the modifications in _get_entry_script_icon for the management of the resource icons, more specifically the calls to ResourceLoader::get_resource_type and EditorFileSystem::get_singleton()->find_file:
image
image

I reused the cache that was there but instead of resetting the cache at each _update_tree, I created a clear_icon_cache method and called it only when an icon script or a resource changes. I probably missed some places where the icons could change!?!

It's really not perfect but I guess the performance should be the same as before #77932.

My tests with 3000 resources:
Before: 4-4.5 seconds
After: 1.5-2 seconds

The time remaining on a save occurs in the Tree, I'm really not sure I should touch that:
image

@Hilderin Hilderin requested a review from a team as a code owner June 14, 2024 04:39
@YeldhamDev YeldhamDev added this to the 4.3 milestone Jun 14, 2024
@akien-mga akien-mga changed the title Fix Noticeable freeze after saving a scene #93104 Fix noticeable freeze after saving a scene Jun 14, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

However, I can't reproduce #93104 on an optimized editor build (optimize=speed lto=full). Saving is quite fast both before and after this PR, although it's slightly faster with this PR. This is also the case with a debug build (saving takes ~800 ms in master, ~700 ms with this PR).

In both cases, the FileSystem dock gets scrolled up when saving the scene. I'd prefer if the existing scroll bar position was kept instead, but that should be looked into in its own PR.

PC specifications
  • CPU: Intel Core i9-13900K
  • GPU: NVIDIA GeForce RTX 4090
  • RAM: 64 GB (2×32 GB DDR5-5800 C30)
  • SSD: Solidigm P44 Pro 2 TB
  • OS: Linux (Fedora 39)

@Hilderin
Copy link
Contributor Author

Hilderin commented Jun 17, 2024

Thanks for testing this PR.
I tested on my system and effectively with an optimized build with optimize=speed lto=full the master version is pretty fast even if my PC is less performant then yours, it's around 1-2sec after the save and before the editor unfreeze. With the PR, it's less then a second.

For the scrollbar, I don't have the same issue about the scroll bar position.

My PC specifications:
CPU: Intel Core i5-11600KF 3.9Ghz
GPU: NVIDIA GeForce RTX 3060
OS: Windows 11

Master version:

Taskmgr_68ui63bvr9.mp4

This PR version:

Taskmgr_hsjTUfLPqD.mp4

@akien-mga akien-mga requested a review from KoBeWi June 18, 2024 12:53
@Hilderin Hilderin force-pushed the fix-freeze-after-save branch 2 times, most recently from acf7a8c to 9772532 Compare June 18, 2024 22:08
@Hilderin
Copy link
Contributor Author

One difference I found with my latest modifications are icons for the script that have a @icon. Before these scripts did not have an icon, now they do because of the line icon_path = file_info->script_class_icon_path; in EditorFileSystem::_update_file_icon_path.

Before:
image

After:
image

Should I removed this line?

@KoBeWi
Copy link
Member

KoBeWi commented Jun 20, 2024

Technically it's consistent with the inspector showing icon in Script field, but personally I find it annoying. I think scripts should show script icon (for their language), not their @icon, which should be reserved for instances of these scripts.

@Hilderin
Copy link
Contributor Author

I tend to agree with you. I'll revert it to what it was before.

@Hilderin
Copy link
Contributor Author

Done!

@akien-mga akien-mga merged commit 31f3969 into godotengine:master Jun 21, 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.

Noticeable freeze after saving a scene
5 participants