godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.57k stars 20.28k forks source link

Reduced tween performance when FileDialog in tree #97214

Open int-72h opened 1 week ago

int-72h commented 1 week ago

Tested versions

System information

Arch Linux, Ryzen 5 5600X

Issue description

I'm currently writing a program that uses Godot as a UI toolkit, and it has quite extensive dynamic theming. As a result, there is/was a large use of tweens between theme overrides to smoothly transition between colours on UI objects:

tween.tween_method(func(x): $Install.add_theme_color_override("font_color",x),base_theme.get_color("font_color","ImportantButton"),Color(pallete["light"]),0.2)
tween.tween_method(func(x): $Verify.add_theme_color_override("font_color",x),base_theme.get_color("font_color","Button"),Color(pallete["lightfg"]),0.2)
tween.tween_method(func(x): $Verify.add_theme_color_override("font_hover_color",x),base_theme.get_color("font_hover_color","Button"),Color(pallete["lightfg"]),0.2)
...
tween.tween_property($BottomPanel.get_theme_stylebox("panel"),"bg_color", Color(pallete["main"]), 0.2)
tween.tween_property($TopPanel.get_theme_stylebox("panel"),"bg_color", Color(pallete["dark"]), 0.2)
tween.tween_property($SidePanel.get_theme_stylebox("panel"),"bg_color", Color(pallete["secondary"]), 0.2)
tween.tween_property($Install.get_theme_stylebox("normal"),"bg_color", Color(pallete["accent"]), 0.2)

This worked fine, and was generally performant - however when I added a FileDialog node (not doing anything with it yet, just its presence in the tree) it seemed to drastically reduce performance. The built-in profilers weren't of much help, they said that it did take >238ms, however it said >100ms without the FileDialog. What I found worked was the following:

func test(x):
    print("%s,%s" % [x,Time.get_ticks_msec()])
    counter += 1
...
tween.tween_method(test,0.0,1.0,0.2)

Since all of the tweens seemed to be consistent in their poor performance, this would see what the difference in performance was more objectively - the more values printed before 1, the smoother in theory it will be. Main project without FileDialog:

0.05208333581686,11222
0.16564667224884,11253
0.36402332782745,11285
0.51945328712463,11316
0.67405831813812,11346
0.8277433514595,11377
0.98136335611343,11408
1,11445

Main project with FileDialog:

0.10600939393044,5991
0.66442108154297,6094
1,6194

The amount of values passed has decreased by over half - which leads to stuttery transitions.

Steps to reproduce

One of the issues with this bug is that its more obvious the more complex a program is. A MRP is, ideally, the exact opposite of a complex program. What I've attached is my attempt to get as small as possible whilst still making the issue visible. This doesn't have as pronounced performance loss, however it's still measurable by the print technique as specified above.

1) Open the project 2) Run it. Use the two mandrill buttons on the side to switch between themes, and look at the console output. 3) Close it and add a FileDialog node. 4) Run it again and notice how there's less iterations, and that the smoothness in the logo and colour transitions is worse.

Minimal reproduction project (MRP)

mrp.zip

int-72h commented 1 week ago

Fishy on the Discord has made me aware of start/end_bulk_theme_override() which will likely increase performance and is probably the optimal way to do this. However this is more of a workaround

AThousandShips commented 1 week ago

I'd say this isn't a workaround, it's the intended way to perform changing multiple theme properties at once, it's a way to deal with the unavoidable costs of changing the theme and propagating that, so I'd say that's the solution to this situation

int-72h commented 1 week ago

I've tried this by adding the functions before/after - zero difference unfortunately.

int-72h commented 1 week ago

I'm going to try profiling the engine, might possibly reveal what's happening.

AThousandShips commented 1 week ago

Will test this myself tomorrow and see if I can find anything

int-72h commented 1 week ago

toast2 There's a ton of extra ThemeOwner::propagate_theme_changed calls. I still haven't figured out why exactly.

int-72h commented 1 week ago

I've figured out why, kind of. The entire red block seems to be purely for FileDialog.

AThousandShips commented 1 week ago

Can confirm the behavior and that the bulk theme overload doesn't seem to solve it, but not sure what exactly is wrong here

int-72h commented 1 week ago

I've figured out why, kind of. The entire red block seems to be purely for FileDialog.

About this; stubbing the _notification in FileDialog doesn't remove the huge block but just a smaller block just for it, as to be expected.

There's also some threads with unknown functions which take significantly longer. Those unknown functions call StringName unrefs mainly.

int-72h commented 1 week ago

I've figured out why, kind of. The entire red block seems to be purely for FileDialog.

The root issue is that theming FileDialog is really really slow. I'm not sure how to fully exclude it from theming as a workaround for now.

Nazarwadim commented 5 days ago

This seems to be a Theme problem, not a FileDialog. You change theme_stylebox which triggers a signal to refresh the entire gui tree. In turn, the FileDialog node consists of many gui nodes, which causes such a difference in performance. I would recommend using an alternative animation in the form of ColorRect and others and not changing the theme_stylebox.

You can also put Theme as a unique resource on nodes that modify theme_stylebox(see video below).

https://github.com/user-attachments/assets/a46cdacb-186b-45a1-a6e0-0dd038635ba7

int-72h commented 5 days ago

Thanks for the advice. I'll give this a try.