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

Improve time to close scene with many 3D gizmos #94698

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

aaronp64
Copy link
Contributor

Changed EditorNode3DGizmoPlugin::current_gizmos from List to HashSet, to avoid having to iterate through all gizmos when ~EditorNode3DGizmo unregisters itself. Fixes the scene closing slowness in #94648

@aaronp64
Copy link
Contributor Author

Didn't notice this earlier, but it looks like HashSet doesn't maintain insert order when an element is removed. If an element is removed, and the last element gets moved from after the index in Joint3DGizmoPlugin::incremental_update_gizmos to before it, it won't be redrawn until the next loop around. Might not be an issue, since it still gets redrawn eventually. Maybe SelfList would be a better option here?

@Hilderin
Copy link
Contributor

Hilderin commented Aug 17, 2024

I tested these modifications and I confirm that it helps a lot with the performance on opening and closing a scene from the editor.

Made a little MRP with 10000 cubes in the scene 100x100x10.tscn:
test-godot-editor-gizmo-performance.zip

Opening the scene:
Godot 4.3 rc3: 10 sec
This PR (in debug): 3-4 seconds

Closing the scene:
Godot 4.3 rc3: 18-20 seconds
This PR (in debug): 2-3 seconds

Careful, don't open another scene before testing because the performance will degrate caused by DynamicBVH::insert.

@Hilderin
Copy link
Contributor

Might not be an issue, since it still gets redrawn eventually. Maybe SelfList would be a better option here?

I think it will be more performant to keep the HashSet since the main performance issue was the lookup and the removal of the gizmo objects. The lookup of thousand of objects in a SelfList will probably be very bad. I'm not concerned with the sort that is not kept when adding or removing gizmos, the timer will eventually redraw all the gizmos, as you said.

Changed EditorNode3DGizmoPlugin::current_gizmos from List to HashSet, to avoid having to iterate through all gizmos when ~EditorNode3DGizmo unregisters itself.
@aaronp64 aaronp64 force-pushed the current_gizmos_hashset branch from 9335a11 to 4d0e2ee Compare August 18, 2024 23:50
Copy link
Contributor

@Hilderin Hilderin left a comment

Choose a reason for hiding this comment

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

Everything sounds good to me.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Aug 31, 2024
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.

Code changes make sense to me.

@akien-mga akien-mga merged commit 2312345 into godotengine:master Sep 2, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronp64 aaronp64 deleted the current_gizmos_hashset branch September 3, 2024 14:51
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.

4 participants