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

Close #19870: Make new colours compatible with UI themes #21991

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Gymnasiast
Copy link
Member

@Gymnasiast Gymnasiast commented May 9, 2024

This refactors the usage of COLOUR_FLAG_* to a new struct with a field for colour and a field for flags. This frees up the upper bits, which were previously clashing with any colours with ID >= 32.

Future refactors left out of this PR:

  • Also refactor out the flags in the drawing engine
  • Change colour_t to an enum (requires this PR and the refactor mentioned above)

@Gymnasiast Gymnasiast marked this pull request as draft May 9, 2024 00:46
@Gymnasiast Gymnasiast linked an issue May 9, 2024 that may be closed by this pull request
@Gymnasiast Gymnasiast changed the title Close #19870: Make new colors compatible with UI themes Close #19870: Make new colours compatible with UI themes May 9, 2024
@Gymnasiast Gymnasiast force-pushed the fix/19870 branch 4 times, most recently from bfd88ea to 0f3f53b Compare May 9, 2024 17:24
@Gymnasiast Gymnasiast marked this pull request as ready for review May 9, 2024 17:24
@733737
Copy link
Contributor

733737 commented May 9, 2024

does this allow the colors to be used for ride names?

@Gymnasiast
Copy link
Member Author

does this allow the colors to be used for ride names?

No, that’s a different set of colours.

@AaronVanGeffen
Copy link
Member

Nice refactor, and good to have the extra colours exposed through the themes API. Things appear to be working properly as well.

I wonder about the serialisation aspect of the implementation, though. Like before, colours are serialised to integer numbers, just longer ones. For example:

{
    "entries": {
        "WC_CHEATS": {
            "colours": [
                257,
                19
            ]
        },
        "WC_CLEAR_SCENERY": {
            "colours": [
                280,
                24,
                24
            ]
        },

I'm wondering whether it would be better to take this opportunity to separate flags from colours as well, e.g.:

{
    "entries": {
        "WC_CHEATS": {
            "colours": [
                1,
                19
            ],
            "flags": [
                1,
                0
            ]
        },
        "WC_CLEAR_SCENERY": {
            "colours": [
                24,
                24,
                24
            ],
            "flags": [
                1,
                0,
                0
            ]
        },

It would be even better if everything used (string) constants, but I think that's overkill for a config that few people modify by hand. I think extracting the flags out would be a nice middle ground, though.

@Gymnasiast
Copy link
Member Author

Gymnasiast commented May 18, 2024

Stuff left to do:

  • Use colour names in theme JSON
  • Update plugin API to use something similar

@Gymnasiast Gymnasiast force-pushed the fix/19870 branch 3 times, most recently from 3dc4b21 to d120878 Compare May 19, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make new colours compatible with UI themes
3 participants