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 slow import when window is unfocused #93953

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Jul 4, 2024

Fixes #93877

Effectively, the importation speed was dependent on the VSync and Update continuously settings. That was because there is a lot of progress popup update while importing scenes. Disabling VSync and Update continuously settings while importing files accelerated the importation process on my computer and I got the same speed now when the editor is focused or not. On my computer, it accelerated the importation of the MRP project from 132 sec. to 88 sec. even when focused.

At the same time, I made a couple of little tweaks in the progress popups to prevent redrawing at each step progress passing p_force_redraw to false to step method.
There was also a bug in ProgressDialog when more then one step was present. If the second task was updated quickly or with p_force_redraw at true, the first step was never updated if the first task was using p_force_redraw at false. That was because last_progress_tick was global and always updated only for the second task. When task_step was called for the first task, last_progress_tick was always up to date causing it to return and not update the step.

@akien-mga akien-mga changed the title Fix slow importation when window is unfocused Fix slow import when window is unfocused Jul 4, 2024
@akien-mga akien-mga requested review from Calinou and KoBeWi July 4, 2024 20:11
@akien-mga akien-mga added this to the 4.3 milestone Jul 4, 2024
@Hilderin Hilderin force-pushed the fix-slow-import-speeds-when-unfocused branch from c77b3bc to 1efeb7e Compare July 4, 2024 20:13
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I didn't really measure any noticeable difference (though I only tested with 10% of the import), but the code looks fine.

But I wonder why does it even depend on framerate. Is it because of Main::iteration() call? Maybe there is a way to call it async somehow.

@Hilderin
Copy link
Contributor Author

Hilderin commented Jul 4, 2024

Yes, I think it's because of Main::iteration(). When VSync is enabled, the DisplayServer waits before rendering the frame (I think) and with low_processor_usage_mode, OS::add_frame_delay adds low_processor_usage_mode_sleep_usec each frame.

@Hilderin Hilderin force-pushed the fix-slow-import-speeds-when-unfocused branch from 1efeb7e to ce7e161 Compare July 4, 2024 23:09
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 mostly works as expected.

Reimporting all resources from the MRP linked in #93877 takes 51 seconds with this PR. I used an optimized editor build for testing, just like in my issue.

The progress dialog clearly redraws very quickly (photosensitivity warning):

fast_redraw.mp4

However, it's not quite as fast as with the "optimal" settings listed in #93877, where I get 12 seconds.

If I set V-Sync mode to Disabled in the Editor Settings with this PR, I get 22 seconds. Enabling Update Continuously has no effect on import times.

I wonder if this has to do with V-Sync only being applied on the main window and not the progress window, or vice-versa.

Edit: Even with current master and optimal settings, I can't get any lower than 22 seconds (matching this PR with V-Sync disabled in the editor settings). Something else may have caused import speeds to regress between 92c8e87 and b97110c – or in fact, between 92c8e87 and 4d984b6, since I didn't rebase your PR when testing it.

@Hilderin
Copy link
Contributor Author

Hilderin commented Jul 7, 2024

Thanks for testing. I'm glad that you are able to get the same performance has the "optional settings".
For the difference in the time for the importation, I took a quick look at all the modifications between 92c8e87 and 4d984b6
https://github.com/godotengine/godot/compare/92c8e87..4d984b6

The only thing I see that could have affected the importation is that commit which was suppose to optimize the load/saving of scenes: b83c64f (#93723)

I don't known, maybe it reloads all the meshes to load scene groups?!? I could look into it a bit later.

@Calinou
Copy link
Member

Calinou commented Jul 7, 2024

I don't known, maybe it reloads all the meshes to load scene groups?!? I could look into it a bit later.

After testing 4.3.beta2, 4.3.dev2 and 4.2.2, I can't reproduce my time of 12 seconds with any of their official builds (or my self-compiled build of 92c8e87). I'm not sure how I managed to get that time in the first place. If anything, the latest master branch is a bit faster than 4.2.2.

@Hilderin
Copy link
Contributor Author

Hilderin commented Jul 7, 2024

That's a good news!!! I'll not investigate further then. Thanks!

@akien-mga akien-mga modified the milestones: 4.3, 4.4 Jul 7, 2024
@akien-mga akien-mga added cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release performance topic:editor topic:import
Projects
None yet
4 participants