Skip to content

Conversation

@frenzibyte
Copy link
Member

Over time, it became tedious and pointless to try providing fallback colours on the assumption that OverlayColourProvider may not be available for some components. To the point ThemeComparisonTestScene was altered to opt-out the "no colour provider" visual case.

After recent discussion, we agreed to provide a default colour scheme at game/test level. I've chosen OverlayColourScheme.Pink as most UI controls usually fall back to pink, and the scheme itself feels more at home than aquamarine.

I've took a spin through the game and there were no visual changes as far as I can see.

@frenzibyte frenzibyte added the code quality Fixes code quality. Not visible to the end user. label Dec 19, 2025
@bdach
Copy link
Collaborator

bdach commented Dec 23, 2025

I'm not super convinced about making this change (subjective gut feeling with no real reasoning), or really super willing to go and check everything in the game for possible visual breakage, so abstaining from review at least for the time being.

If I were to try and vocalise why I don't like this, I foresee breakage with this such as "popover X is part of screen Y that caches its screen-level colour provider, but because of technicalities popover X ends up loading outside screen Y's hierarchy, and is given the game-global colour provider with a completely different hue". Right now that'll at least loudly die instead of silently looking wrong. Not a failure mode that's unique to popovers either I think.

@frenzibyte
Copy link
Member Author

I anticipated you wouldn't like this change purely due to its scope, so I'll detach it from the dependent PR above.

That being said, I think we all agree continuing to pass arbitrary colour fallbacks in UI components that should never be instantiated without a colour provider is awkward to have.

Special cases like tooltips and popovers can be handled individually, through methods like enforcing OverlayColourProvider to be passed through constructor from the drawable instantiating it.

@peppy
Copy link
Member

peppy commented Dec 25, 2025

That being said, I think we all agree continuing to pass arbitrary colour fallbacks in UI components that should never be instantiated without a colour provider is awkward to have.

An alternative is to remove the fallbacks and fix the cases one by one where it's missing. That would avoid scenarios that @bdach touches on where colour mismatches occur (potential actual hidden regressions from this PR, as the resultant colour may clash harder than the neutral fallbacks).

@frenzibyte
Copy link
Member Author

frenzibyte commented Dec 25, 2025

Though doing this comes at the cost of users potentially facing game crashes instead of just incorrect visuals. This PR is written to favour the latter over the former.

@peppy
Copy link
Member

peppy commented Dec 25, 2025

Though doing this comes at the cost of users potentially facing game crashes instead of just incorrect visuals. This PR is written to favour the latter over the former.

One would hope we have tests covering such cases which would crash for us, not users.

@frenzibyte
Copy link
Member Author

Fair enough. I'll close this PR and adjust #36073 accordingly.

@frenzibyte frenzibyte closed this Dec 25, 2025
@frenzibyte frenzibyte deleted the default-overlay-colour-provider branch December 25, 2025 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality Fixes code quality. Not visible to the end user. size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants