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

Return fast for built-in class icon #101435

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

YYF233333
Copy link
Contributor

@YYF233333 YYF233333 commented Jan 11, 2025

Add a fast path for built-in class in EditorNode::get_class_icon.

#78645 MRP with 28K objects now runs smoothly, 91K objects still sucks.

Would be good if someone knows what EditorNode::get_object_icon does could refactor these code.

Refactor get icon logic.

  • Split _get_class_or_script_icon into _get_class_icon and _get_script_icon.
  • Change get_class/object_icon to avoid multiple checking.
  • Use file extension to identify resource path. This should be cheaper than ResourceLoader::exists.

Test with test_custom_icon.zip, class_name and @icon seems to work without problem. And the performance boost remains true.

Bugsquad edit:

@YYF233333 YYF233333 changed the title Return fast for built-in class icon. Return fast for built-in class icon Jan 11, 2025
@Calinou Calinou added this to the 4.4 milestone Jan 11, 2025
@YYF233333 YYF233333 requested a review from a team as a code owner January 12, 2025 14:48
@YYF233333 YYF233333 changed the title Return fast for built-in class icon Refactor EditorNode get icon Jan 12, 2025
@akien-mga akien-mga requested a review from KoBeWi January 12, 2025 16:45
@777joe
Copy link

777joe commented Jan 12, 2025

Tried this out. This fixes #98666 too.

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. It's faster overall but due to remaining bottlenecks, scrolling large Trees is still sluggish, especially when near the bottom.

Code looks good to me.

All tests done with an optimized editor build with theMRP from #78645.

I've also tried the MRP of this PR with 16,384 nodes (duplicated from the original 4 nodes) but couldn't spot much of a performance difference when scrolling.

Local

master

master_local.mp4

This PR

pr_local.mp4

Remote

Stutters due to Tree updates being performed every second are still present.

master

master_remote.mp4

This PR

pr_remote.mp4

@KoBeWi
Copy link
Member

KoBeWi commented Jan 24, 2025

This looks mostly like a cleanup, while #101489 is a complete solution. The key point is avoiding to load scripts.

Still, it does not have problems I mentioned in #101489 (comment), and if it's indeed faster, then it's fine for now. The two PRs don't seem mutually exclusive.

@YYF233333 YYF233333 force-pushed the remote_tree_fix2 branch 3 times, most recently from a454986 to 30cf85b Compare January 27, 2025 08:57
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Code looks good

Co-authored-by: Tomasz Chabora <[email protected]>
@YYF233333
Copy link
Contributor Author

YYF233333 commented Feb 4, 2025

Unfortunately I cannot make this refactor after #101489, it will crash the editor when create node for unknown reason. But at least we can solve the performance problem first.

I also found a possible regression: after creating a new script and adding @icon, the icon doesn't load—it requires restarting the editor to appear. Don't know if it is related to #101489. I tested this on the dev version, not sure if the release version is affected as well.

Edit: This behaviour exists before #101489. (I kind of remember last month I do this test it doesn't require a scene reload but, anyway, not a big issue.)

@KoBeWi
Copy link
Member

KoBeWi commented Feb 4, 2025

after creating a new script and adding @ICON, the icon doesn't load

It should refresh when you close and re-open the scene. Script icon cache is only invalidated when any scene is closed (though not sure if it's still the same after #101489).

@YYF233333 YYF233333 changed the title Refactor EditorNode get icon Return fast for built-in class icon Feb 5, 2025
@akien-mga akien-mga added the bug label Feb 10, 2025
@Repiteo
Copy link
Contributor

Repiteo commented Feb 10, 2025

Thanks!

@YYF233333 YYF233333 deleted the remote_tree_fix2 branch February 11, 2025 09:49
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.

GDScript code completion is extremely laggy on Android Editor
8 participants