Skip to content

Fix noticeable freeze after saving a scene - #93147

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
Hilderin:fix-freeze-after-save
Jun 21, 2024
Merged

Fix noticeable freeze after saving a scene#93147
akien-mga merged 1 commit into
godotengine:masterfrom
Hilderin:fix-freeze-after-save

Conversation

@Hilderin

Copy link
Copy Markdown
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 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
@Hilderin
Hilderin force-pushed the fix-freeze-after-save branch from 03115cd to 4dfd741 Compare June 15, 2024 01:06

@Calinou Calinou left a comment

Copy link
Copy Markdown
Member

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

Hilderin commented Jun 17, 2024

Copy link
Copy Markdown
Contributor Author

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
Comment thread editor/editor_file_system.cpp Outdated
Comment thread editor/editor_file_system.cpp Outdated
Comment thread editor/editor_file_system.cpp Outdated
@Hilderin
Hilderin force-pushed the fix-freeze-after-save branch 2 times, most recently from acf7a8c to 9772532 Compare June 18, 2024 22:08
Comment thread editor/editor_file_system.cpp Outdated
Comment thread editor/editor_file_system.cpp Outdated
Comment thread editor/editor_file_system.cpp Outdated
@Hilderin
Hilderin force-pushed the fix-freeze-after-save branch from 9772532 to de0f6bd Compare June 20, 2024 20:20
@Hilderin

Copy link
Copy Markdown
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

KoBeWi commented Jun 20, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@Hilderin
Hilderin force-pushed the fix-freeze-after-save branch from de0f6bd to f1aafa2 Compare June 20, 2024 20:40
@Hilderin

Copy link
Copy Markdown
Contributor Author

Done!

Comment thread editor/editor_file_system.cpp Outdated
@Hilderin
Hilderin force-pushed the fix-freeze-after-save branch from f1aafa2 to 8ac841a Compare June 20, 2024 20:47
@akien-mga
akien-mga merged commit 31f3969 into godotengine:master Jun 21, 2024
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
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