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

Add boot splash minimum display time setting #41833

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Sep 7, 2020

Implements #8867.


By default, display time is 0 (equivalent to current behavior).

I found this useful because simple games start so quickly that the splash screen blinks and you can't see anything. The recommended delay is 500 ms – 1 s.

https://github.com/godotengine/godot/blob/b592b74d66f8d728cc512bbde5448d9f4c4e1624/main/main.cpp#L1596

delay_usec is used instead of delay_msec because delay_msec is defined in core_bind.cpp and is not yet available in main.cpp.

This PR is cherry-pickable for 3.2.

@ghost

This comment has been minimized.

@dalexeev
Copy link
Member Author

dalexeev commented Sep 7, 2020

the engine should load AS FAST AS POSSIBLE

By default, it is. This PR adds the ability to set a small delay. This is not intended to make you watch the splash screen for 10 seconds. I also don't like long splash screens. It might be worth adding a limit or warning so people don't set the delay too long.

On fast devices, simple games start so quickly that you don't have time to see anything, the splash screen literally blinks. It's not very pretty. If you set a delay of 500 ms, then it will not delay you much, but it will give you a chance to at least see what is shown on the splash screen.

@akien-mga
Copy link
Member

@RaTcHeT302 I'll have to ask you again to pay attention to your language and make sure that your feedback on Godot community channels respects our Code of Conduct, and namely:

  • Politeness is expected at all times. Be kind and courteous.
  • Always assume positive intent from others. Be aware that differences in culture and English proficiency make written communication more difficult than face-to-face communication and that your interpretation of messages may not be the one the author intended. Conversely, if someone asks you to rephrase something you said, be ready to do so without feeling judged.
  • Feedback is always welcome but keep your criticism constructive. We encourage you to open discussions, proposals, issues, and bug reports. Use the community platforms to discuss improvements, not to vent out frustration. Similarly, when other users offer you feedback please accept it gracefully.

As you were warned repeatedly but you keep offending, I'm blocking you for interacting with @godotengine repository for 7 days. Please be more polite and constructive in future interactions once this temporary ban is lifted, or the next step would be a permanent ban.

@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:core labels Sep 7, 2020
@Calinou Calinou added this to the 4.0 milestone Sep 7, 2020
@Chaosus
Copy link
Member

Chaosus commented Sep 7, 2020

I think the potential user will use custom/multiply splash screens with possible animation with a much longer delay (as in many games today) so this change is harmless (if this can be configurable to 0 in the editor settings).

@ghost
Copy link

ghost commented Sep 8, 2020

Minimum display time is more accurate than delay time. Delays are periods of waiting before something happens.

Seems like a good option for those that only use the boot splash for startup information.

Seeing the msec comparison, I'm curious how long the boot image has been rendered by that point. Is it really going to be visible for all the ticks during setup?

@dalexeev
Copy link
Member Author

dalexeev commented Sep 9, 2020

@avencherus Yes, that's a good name. I gave the name by analogy with application/run/frame_delay_msec.

As I understand it, OS::get_singleton()->get_ticks_msec() is the time since the start of the process, not since the game window appeared. So, probably, this feature can be improved.

main/main.cpp Outdated Show resolved Hide resolved
@dalexeev dalexeev requested a review from a team as a code owner September 21, 2020 13:32
@dalexeev dalexeev changed the title Add minimum delay setting for boot splash Add boot splash display time setting Sep 21, 2020
@dalexeev
Copy link
Member Author

main/main.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

By the way, isn't this a mistake?

https://github.com/godotengine/godot/blob/b592b74d66f8d728cc512bbde5448d9f4c4e1624/main/main.cpp#L1581-L1582

rendering/environment/default_clear_color and application/boot_splash/bg_color are different settings.

It looks OK to me. This line is about setting the clear color for the boot splash, which can be customized via application/boot_splash/bg_color (e.g. if you want to have your logo with alpha background on a custom background color).

Once the boot splash has been shown, the default clear color is then set to rendering/environment/default_clear_color which is the value used for the game itself. Most of the time it will be different from the boot splash background color.

https://github.com/godotengine/godot/blob/b592b74d66f8d728cc512bbde5448d9f4c4e1624/main/main.cpp#L1605-L1607

@akien-mga
Copy link
Member

akien-mga commented Sep 24, 2020

Also OS::get_singleton()->_msec_splash is not set if default boot splash is used.

That sounds like a bug indeed.
Then I don't see what's the point of _msec_splash / OS::get_splash_tick_msec() in the first place.

It was added in 3e20391, not sure why.

Edit: I propose removing it in #42304.

@dalexeev
Copy link
Member Author

It looks OK to me. <...> Most of the time it will be different from the boot splash background color.

Yes, it works right. I am confused because the GLOBAL_DEF macro is used 2 times:

https://github.com/godotengine/godot/blob/b592b74d66f8d728cc512bbde5448d9f4c4e1624/main/main.cpp#L1542-L1543

https://github.com/godotengine/godot/blob/b592b74d66f8d728cc512bbde5448d9f4c4e1624/main/main.cpp#L1605-L1608

and the code below looks like the boot_bg_color variable is being applied twice:

https://github.com/godotengine/godot/blob/b592b74d66f8d728cc512bbde5448d9f4c4e1624/main/main.cpp#L1581-L1584


I found an additional problem: the default icon is displayed during boot splash. Is it possible to move the icon initialization code from the start function to the setup2 function? The last function contains the corresponding GLOBAL_DEFs, but they are not used:

https://github.com/godotengine/godot/blob/b592b74d66f8d728cc512bbde5448d9f4c4e1624/main/main.cpp#L1610-L1625

and used here:

https://github.com/godotengine/godot/blob/b592b74d66f8d728cc512bbde5448d9f4c4e1624/main/main.cpp#L2266-L2325

@Calinou
Copy link
Member

Calinou commented Mar 20, 2021

I'm not really convinced of the utility of displaying the "dumb" splash screen for a longer duration. It has no possibilities for animation, fade-in effects or anything of the sort.

When you want to create a polished splash setup, you generally use a plain color as the splash screen and then set a splash scene as your project's main scene (with its starting color matching the plain color chosen).

doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
main/main.cpp Outdated Show resolved Hide resolved
@reduz
Copy link
Member

reduz commented Aug 31, 2021

I understand why this feature is needed, but the way it is implemented is not good. It should be a minimum display time. This means that if the engine initialized and loaded the main scene too fast, it should wait until this time has elapsed.

For this, measuring the initialization time is needed, and then see if waiting more time is needed.

@dalexeev dalexeev requested a review from a team as a code owner September 2, 2021 20:30
@dalexeev dalexeev changed the title Add boot splash display time setting Add boot splash minimum display time setting Sep 2, 2021
@akien-mga akien-mga requested a review from reduz November 23, 2021 14:59
@mhilbrunner
Copy link
Member

mhilbrunner commented Nov 23, 2021

We reviewed this in the current PR meeting. Its wanted and implementation looks good, seems @reduz feedback was implemented, thansk for that :)

The code should be wrapped in an if clause to check whether the setting is set and just do nothing if it wasn't/is set to 0 just for clarity.

Otherwise, maybe @lawnjelly wants to give it a final look as timing expert :P But should be good to go otherwise

@dalexeev
Copy link
Member Author

dalexeev commented Nov 23, 2021

There are 2 problems:

  1. Time should be counted from the moment the window appears, and not from the moment the process starts.
  2. As far as I remember, the default Godot icon is not immediately replaced by the custom one (at least in KDE). But, this is not directly related to this PR, because the icon was replaced during OS.delay_usec (which is weird).

For myself, I solved the problem by using the image of a small hourglass. That is, it is more of a boot image than a splash image.

I understand why people supported this PR, but now it doesn't seem like a good idea to me. Either leave it as it is and add a clarification that the splash image is actually a boot image, which should have an hourglass icon, the text "Loading..." or something similar, and not logos. Or implement this feature, but more thoroughly than in this PR.

main/main.cpp Outdated
PropertyInfo(Variant::INT,
"application/boot_splash/minimum_display_time_msec",
PROPERTY_HINT_RANGE,
"0,100,1,or_greater")); // No negative numbers
Copy link
Member

Choose a reason for hiding this comment

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

You can add suffix:ms and remove _msec from the name I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: We should rename other similar settings.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the suffix is a new feature, we could use it consistently for this kind of setting/property.

main/main.cpp Outdated Show resolved Hide resolved
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.

Approved in PR review meeting, though there's still an issue to solve with the docs.

@dalexeev dalexeev force-pushed the splash_delay branch 2 times, most recently from bef9d7d to 6b6ebaf Compare June 28, 2022 15:49
main/main.cpp Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

This was fully approved during the PR meeting, thanks for your work!

@YuriSizov YuriSizov merged commit 42537da into godotengine:master Jul 12, 2022
@dalexeev dalexeev deleted the splash_delay branch July 12, 2022 16:11
@akien-mga
Copy link
Member

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 24, 2022
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.

9 participants