godotengine
Display project settings splash color on web export
This is a minor improvement to the new web export splash screen, which now displays the correct splash color specified in the project settings as the background, as was mentioned in #91852.
I've been a fan of Godot for quite some time now and have been using it for hobby projects, as well as convincing friends to try it. I don't have much experience with the inner workings of the engine, or C++ for that matter but I got very excited to find that I can help out with small additions too ;). My changes have been tested locally and work as expected but since this is my very first contribution to Godot, I'd appreciate a thorough check and I hope that everything is alright!
- Bugsquad edit, closes: https://github.com/godotengine/godot/issues/96874
We can keep $GODOT_SPLASH as-is, or provide $GODOT_SPLASH along with $GODOT_SPLASH_IMAGE for compatibility.
Both can work, but keeping $GODOT_SPLASH the way it is requires a specific order in the replaces HashMap for it not to break the $GODOT_SPLASH_COLOR placeholder. I think it is also more intuitive to name them more similarly to the project settings they apply, so if we want to keep the compatibility, providing it alongside $GODOT_SPLASH_IMAGE might be the better way to go.
Has there been any decision yet as to how we should proceed regarding compatibility? I'm happy to make any changes needed.
I don't understand how it could break.
If $GODOT_SPLASH is replaced before $GODOT_SPLASH_COLOR, it changes the "$GODOT_SPLASH_COLOR" string in the CSS to e.g. "splashimage.png_COLOR", breaking the value and preventing the correct replacement (since the placeholder string doesn't exist in the CSS anymore). I just realised my initial assessment was likely wrong too (though I can't test it currently) - changing the order in the code wouldn't reliably work either, since we're dealing with a HashMap. If we want to revert back to the old name, we would either have to rename $GODOT_SPLASH_COLOR so that $GODOT_SPLASH is no longer a prefix, or change the implementation of the replacement.
Even if I like up-to-date code, it doesn't warrant breaking compatibility, really.
I agree! I was under the impression that strict compatibility preservation is only done for the API, not for the export system, which is why I didn't think of this problem.
Sorry for the delay! Don't hesitate to join the developers' chat #Web channel in order to discuss topics.
Done, much appreciated!
I just realised my initial assessment was likely wrong too (though I can't test it currently) - changing the order in the code wouldn't reliably work either, since we're dealing with a HashMap.
I was able to test this right now and skimmed over the implementation too, and I believe Godot's HashMap iteration actually does return elements in the order they were added. So the options I currently see are the following:
- Keep the rename, highlight it as a breaking change for custom HTML shells (I'd prefer this, as it follows a clear, understandable naming scheme similar to the project settings)
- Keep the old name, change the
$GODOT_SPLASH_COLORplaceholder to something else (maybe$GODOT_COLOR_SPLASH? Seems inconsistent to me though) - Keep the old name, change the order so that
$GODOT_SPLASHis after$GODOT_SPLASH_COLOR - Keep the old name, rework the replacement logic to not replace a string if there is a non-terminating character after the match
I just realised that my rebase seems to have caused review requests to be sent to all code owner teams affected by changes since I opened this PR - I'm very sorry for this!
- Keep the old name, change the order so that
$GODOT_SPLASHis after$GODOT_SPLASH_COLOR
Yes. This is the simplest solution.
Alright, I made the change now! I hope we can eventually move to a variable name that matches the project settings since relying on the order seems a little fragile to me but for now, I think this is good to merge! Let me know if there's anything else I should change and thank you for taking the time to review this PR. :)
