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 performance issue reimport file reload scene #95225

Merged

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Aug 7, 2024

This PR should significantly improve performance when reimporting assets with the auto-reload of scenes.

What has been done:

  • Addition and removal of the reloaded scene in EditorNode::preload_reimporting_with_path_in_edited_scenes: It takes 7-8 seconds on my PC just to remove and re-add the scene. This part should not require the scene to be in the tree to load the modified properties and nodes to reimport. I had to modify EditorNode::get_preload_scene_modification_table to use get_transform instead of get_relative_transform. I don't know why get_relative_transform was used, but in my tests, it works fine with get_transform. Additionally, when the transform is restored, it uses set_transform.
  • Adjustment in EditorNode::reload_instances_with_path_in_edited_scenes: The addition and removal of the reloaded scene have been minimized. The current edited scene should almost never be removed unless another scene that needs to be reloaded has the same name.
  • Modification in EditorNode::_set_current_scene_nocheck: The addition and removal of the reloaded scene have been removed if the same scene is reloaded.
  • Replacement of EditorNode::update_node_reference_modification_table_for_node: This method has been replaced by EditorNode::get_preload_modifications_reference_to_nodes, which now runs in the preload and loads only modified properties that reference reimported nodes. Previously, it loaded every property modification in the scene, taking about 10 seconds. Now, it takes around 7 seconds in debug mode. The main issue was the call to p_node->get_property_list for each node in the scene. I have not found a better solution to optimize this.
  • Added progress popups: These display visual feedback to the user before and during the reloading process.

Tested

Known issues

  • On my PC, opening Godot on startup with the MRP is very slow. Commenting out the method Node3DEditor::_request_gizmo removes 15 seconds from the startup time.
  • I created issue Signal editor_description_changed not disconnected on SceneTreeDock refresh #95221 for signals that never disconnect and accumulate on nodes.
  • Adding and removing scenes with many nodes is slow. Just switching scene tabs can take 5-6 seconds. Most of the time is spent in add_child and remove_child.

I will not work on these issues for now, as I think the current performance of this PR is acceptable.

Time

Reloading an asset in the MRP (debug dev build):

  • Before: 5 min. +
  • Now: 10-12 sec.

Reloading an asset in the MRP (prod build):

  • Before (4.3 rc2): 3 min. +
  • Now (artifact from the PR): 4-6 sec.

With the artifact from the PR:

godot.windows.editor.x86_64_LIsqklfJDn.mp4

@Calinou
Copy link
Member

Calinou commented Aug 7, 2024

  • On my PC, opening Godot on startup with the MRP is very slow. Commenting out the method Node3DEditor::_request_gizmo removes 15 seconds from the startup time.

See #94648.

@akien-mga
Copy link
Member

Thanks so much for working on this @Hilderin, you're once again saving the 4.3 release 🥇
This regression is the main release blocker left IMO, so once we have this merged we should hopefully be good to go 🤞

I'll try to give this a quick test and superficial review, and aim to merge it today so I can release 4.3.rc3 tomorrow as hopefully the "final FINAL v3 final.psd" RC!

@akien-mga
Copy link
Member

Modifying all .gltf files to add a space at the end and thus trigger a reimport (for file in $(find -name "*.gltf"); do echo " " >> $file; done):

Current master: 13 s 870 ms (dev build), 5 s 380 ms (optimized build)
This PR: 5 s 410 ms (dev build), 3 s 870 ms (optimized build)

  • Test with (private, for now) W4 demo Project Genesis, modifying the massive FortifiedCastleTown.gltf file to add an empty space and trigger a reimport.

Current master: More than 8 min, I gave up (optimized build)
This PR: 1 min 34 s (optimized build)

Seems pretty drastic!

I've also done a cursory review of the code, looks fine to me overall, but I'm no expert in this part of the engine.

@clayjohn
Copy link
Member

clayjohn commented Aug 7, 2024

I have tested this PR and confirm that it fixes the performance and brings things in line with expectations (3+ minutes down to 7 seconds).

On my device, initial import of the entire project is about 45 seconds (25 seconds to import + 20 seconds to open the scene).

On master re-importing a single mesh took 3+ minutes and with this PR the one i tested took 7 seconds.

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.

Approving based on my above test and quick review.

Clay also mentioned having very good results with the MRP and an optimized build.

@akien-mga akien-mga merged commit 4bef4d9 into godotengine:master Aug 7, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks! Amazing work once again 🎉

@Hilderin
Copy link
Contributor Author

Hilderin commented Aug 8, 2024

Thanks!! Unfortunately, I found a problem in this PR when multiple files are reimported at the same time. I created a small PR to fix that: #95264

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.

Severe regression in asset import speeds
5 participants