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 3 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 5 times, most recently from ea97f71 to 499080a Compare May 20, 2024 14:10
Basssiiie

This comment was marked as outdated.

@Gymnasiast

This comment was marked as outdated.

@Gymnasiast
Copy link
Member Author

I have split off the plugin stuff from this PR and instead emulated old behaviour. This will be revisited at a later date.

Copy link
Member

@AaronVanGeffen AaronVanGeffen left a comment

Choose a reason for hiding this comment

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

Testing found no abnormalities. I like the improved JSON format as well, with colours now being stored as strings, and transparency being a boolean. Makes these theme config files way more readable. Nice work!

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
4 participants