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

Create ColorPicker shaders statically #48690

Merged
merged 1 commit into from
May 17, 2021

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented May 13, 2021

As discovered by @pycbouh, the recent inspector slowdows were introduced by #46340. Turns out that each ColorPicker was compiling shaders when it was created, which was terribly slow.

This PR moves the shader compilation to a static method, so it's done only once.

@KoBeWi KoBeWi added this to the 4.0 milestone May 13, 2021
@KoBeWi KoBeWi requested a review from a team as a code owner May 13, 2021 13:55
@YuriSizov
Copy link
Contributor

YuriSizov commented May 13, 2021

Definitely alleviates some of the slowdown. There are likely some other causes unrelated to #46340 too, but this is good.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 14, 2021

Any ideas why the checks failed and how to fix this? Seems like tests have problems with instancing the shader.

@groud
Copy link
Member

groud commented May 14, 2021

It seems like there is a segfault. I would try running the unit tests locally and see what causes the bug.

 ERROR: Invalid locale 'C'.
   at: get_language_code (core/string/translation.cpp:957)
ERROR: Unsupported locale 'C', falling back to 'en'.
   at: set_locale (core/string/translation.cpp:980)
scene/resources/shader.cpp:155:59: runtime error: member access within null pointer of type 'struct RenderingServer'
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] ./bin/godot.linuxbsd.tools.64s() [0x1fc8878] (/home/runner/work/godot/godot/platform/linuxbsd/crash_handler_linuxbsd.cpp:54)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7f1529b57210] (??:0)
[3] Shader::Shader() (/home/runner/work/godot/godot/scene/resources/shader.cpp:155 (discriminator 1))
[4] Ref<Shader>::instance() (/home/runner/work/godot/godot/./core/object/reference.h:227)
[5] ColorPicker::initialize_shaders() (/home/runner/work/godot/godot/scene/gui/color_picker.cpp:92)
[6] register_scene_types() (/home/runner/work/godot/godot/scene/register_scene_types.cpp:966)
[7] Main::test_setup() (/home/runner/work/godot/godot/main/main.cpp:422)
[8] Main::test_entrypoint(int, char**, bool&) (/home/runner/work/godot/godot/main/main.cpp:486)
[9] ./bin/godot.linuxbsd.tools.64s(main+0x144) [0x1fc5efa] (/home/runner/work/godot/godot/platform/linuxbsd/godot_linuxbsd.cpp:45)
[10] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7f1529b380b3] (??:0)
[11] ./bin/godot.linuxbsd.tools.64s(_start+0x2e) [0x1fc5cfe] (??:?)
-- END OF BACKTRACE --

@KoBeWi
Copy link
Member Author

KoBeWi commented May 14, 2021

Well I tried and I have no idea. It's like Ref is not initialized, but I tried creating a local variable and it doesn't work either. memnew also doesn't help.

@akien-mga
Copy link
Member

The crash happens in the test codepath (Main::test_entrypoint which does not initialize the RenderingServer). Apparently this addition of static Shaders in scene is a first and adds a dependency for the RenderingServer to be fully initialized before nodes, which might be problematic.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 17, 2021

Ok, fixed. I also refactored it a bit, because I noticed that other classes create some shaders too.

@akien-mga akien-mga merged commit 9cc17a8 into godotengine:master May 17, 2021
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the static_shader_picker branch May 17, 2021 12:52
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