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

The initialization window matches the editor base color or splash background color #63819

Closed
wants to merge 1 commit into from

Conversation

CodeLazier
Copy link

@CodeLazier CodeLazier commented Aug 2, 2022

This pr is a graft for #53922, due to the fact that #53922 has a lot of conflicts for a long time, and the code is modified as required. pr has been manually corrected to can be merged correctly.

Bugsquad edit: This closes #58056.

main/main.cpp Outdated Show resolved Hide resolved
servers/display_server.h Outdated Show resolved Hide resolved
main/main.cpp Outdated
@@ -261,6 +262,27 @@ void finalize_navigation_server() {
navigation_server_2d = nullptr;
}

// Take the configuration value before the editor initialization.
Copy link
Member

Choose a reason for hiding this comment

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

I would move this to a static function in EditorNode

@reduz
Copy link
Member

reduz commented Aug 2, 2022

Looks superb! Thanks for redoing it!

@akien-mga
Copy link
Member

Can this be implemented on platforms other than Windows?

@bruvzg
Copy link
Member

bruvzg commented Aug 2, 2022

Can this be implemented on platforms other than Windows?

Window BG color can be set on macOS, but it's never visible.

@CodeLazier
Copy link
Author

Can this be implemented on platforms other than Windows?

I work on Windows, and the publishing platform is also Windows. The white screen is also the most obviously on Windows.If this problem occurs then on other platforms, then this pr should serve as a helpful foundation.

@CodeLazier CodeLazier force-pushed the master branch 5 times, most recently from 71dd520 to bcc34d1 Compare August 2, 2022 15:59
@Calinou
Copy link
Member

Calinou commented Aug 2, 2022

Godot's X11 platform doesn't initially display any color when creating a window, so I think it's fine to have this only for Windows for now.

* This static method is used during the initialization phase of the initial window,
* care should be taken to detect whether each instance of the class is available.
*/
EditorPaths *p_editorPaths = EditorPaths::get_singleton();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EditorPaths *p_editorPaths = EditorPaths::get_singleton();
EditorPaths *editor_paths = EditorPaths::get_singleton();

We use p_ as prefix for function parameters, not for pointers. And we use snake case for identifiers.

Comment on lines 5028 to 5029
// Default black(0,0,0,1)
Color result;
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't document what the default color is, if this ever changes it will make the comment obsolete.
Moreover, you never assign result so it's not relevant to define here, you can just do return Color(); in the fallback case.

main/main.cpp Outdated
Comment on lines 1726 to 1729
#endif
/* Initialize Input */

Copy link
Member

Choose a reason for hiding this comment

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

Keep a separation line between sections of code.

main/main.cpp Outdated
Comment on lines 1906 to 1933
const Color boot_bg_color =
boot_bg_color =
GLOBAL_DEF_BASIC("application/boot_splash/bg_color",
(editor || project_manager) ? boot_splash_editor_bg_color : boot_splash_bg_color);
#else
const Color boot_bg_color = GLOBAL_DEF_BASIC("application/boot_splash/bg_color", boot_splash_bg_color);
boot_bg_color = GLOBAL_DEF_BASIC("application/boot_splash/bg_color", boot_splash_bg_color);
Copy link
Member

Choose a reason for hiding this comment

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

You duplicated this code earlier, is this still needed?

main/main.cpp Outdated
Comment on lines 1744 to 1745
// Godot Default background color.
Color init_bg_color = Color(0.21f, 0.24f, 0.29f);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too fond of hardcoding this, this is bound to get out of sync if we ever change the default background color.

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.

Overall I'm not too fond of these changes personally, I find this adds significantly complexity to the setup for what seems to be a fairly niche need on Windows only. Having to parse editor config before the editor settings have been initialized just to retrieve a configurable theme option seems hacky.

For the running project, we have boot_bg_color, isn't that sufficient?

Wouldn't it be simpler to just hardcode a black color in the Windows window creation instead of doing all this? Or just delay showing the window until it has something to render?

@akien-mga akien-mga removed the request for review from a team August 8, 2022 08:49
@Calinou
Copy link
Member

Calinou commented Aug 8, 2022

Wouldn't it be simpler to just hardcode a black color in the Windows window creation instead of doing all this? Or just delay showing the window until it has something to render?

Either of these approaches sound good to me. A black flash is way less problematic than a white flash in a dark environment.

@CodeLazier
Copy link
Author

CodeLazier commented Aug 8, 2022

Overall I'm not too fond of these changes personally, I find this adds significantly complexity to the setup for what seems to be a fairly niche need on Windows only. Having to parse editor config before the editor settings have been initialized just to retrieve a configurable theme option seems hacky.

For the running project, we have boot_bg_color, isn't that sufficient?

Wouldn't it be simpler to just hardcode a black color in the Windows window creation instead of doing all this? Or just delay showing the window until it has something to render?

If feel that too many changes will not bring benefits, then you can see #53806 , only one line of code has been added.
I will also turn off this pr

@reduz
Copy link
Member

reduz commented Aug 11, 2022

I really apologize that this this idea only comes up now, but would it not be simpler to just define "application/boot_splash/bg_color" earlier on? (We could put the boot splash stuff in ProjectSettings instead of Main), and then simply get it via GLOBAL_GET in the platforms that support this and set it? I think it may be simpler in the end and only affect the platforms where it can actually be done, and no code in main/ needs to change (in fact it will be simpler because we remove the GLOBAL_DEFS)

@CodeLazier
Copy link
Author

CodeLazier commented Sep 3, 2022

sorry,there may not be much time recently. I modified as suggested and found that if you put the get/set color code into display_server_windows.cpp, it will cause some problems. First, you cannot use the default definition of boot_splash_bg_color, and secondly if you want to use the default color defined by the editor theme, then To introduce editor_node.h, my local test will compile errors. If peek_cfg_theme_color() is implemented in display_server_windows.cpp, it will introduce other .h files more than necessary, causing pollution. Therefore, various preset colors must be obtained in main.cpp In this case, the parameters of the virtual function are still added. So it is of little significance. Unless the editor theme bgcolor is no longer obtained, but the effect may be abrupt in the editor (especially the splash bgcolor with high contrast is set and editor theme).

Maybe give me some tips, otherwise it might take some time to see if there is a better solution.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 10, 2023
@akien-mga
Copy link
Member

Superseded by #71289.

@akien-mga akien-mga closed this Feb 22, 2023
@aaronfranke aaronfranke removed this from the 4.x milestone Feb 28, 2023
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.

Editor windows briefly flash white upon opening
7 participants