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 hex and RGB(A) support to all map color attributes #1258

Closed
patrikjuvonen opened this issue Feb 22, 2020 · 6 comments · Fixed by #1261
Closed

Add hex and RGB(A) support to all map color attributes #1258

patrikjuvonen opened this issue Feb 22, 2020 · 6 comments · Fixed by #1261
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@patrikjuvonen
Copy link
Contributor

patrikjuvonen commented Feb 22, 2020

Is your feature request related to a problem? Please describe.
It seems various color formats like hex or RGB(A) aren't supported for many element types like radararea and blip but throw an error instead.

This is a breaking change. For example the race gamemode depends on this old behavior as described in #1393

Describe the solution you'd like
Make color format support consistent across element types.

Describe alternatives you've considered
Use of a third-party script to convert unsupported color values in map files to a supported format.

Additional context

@patrikjuvonen patrikjuvonen added the enhancement New feature or request label Feb 22, 2020
@patrikjuvonen patrikjuvonen self-assigned this Feb 22, 2020
@patrikjuvonen patrikjuvonen added this to the Backlog milestone Feb 22, 2020
@patrikjuvonen patrikjuvonen changed the title Add hex and RGBA support to map color attributes Add mixed hex and RGBA support to map color attributes Feb 22, 2020
@patrikjuvonen patrikjuvonen changed the title Add mixed hex and RGBA support to map color attributes Add mixed hex and RGB(A) support to map color attributes Feb 22, 2020
@qaisjp
Copy link
Contributor

qaisjp commented Feb 23, 2020

Are legacy vehicle colors supported?

@qaisjp
Copy link
Contributor

qaisjp commented Feb 23, 2020

and are you 100% that allowing different types of colors in a single attribute is a good idea?

@patrikjuvonen
Copy link
Contributor Author

patrikjuvonen commented Feb 23, 2020

By legacy colors you probably mean the palette colors. Yes, they are still supported same as before. This is also tested in the provided map file above.

As for mixed values, I think it is trivial, but to be honest I don't see any issues with having it either.

This is actually an "unintended" side-effect of the simplified C++ code to be able to read colors despite odd types mixed together. I didn't want to limit it to a single type. But I think it is not a bad side-effect as it doesn't break anything and is logical. I think it could be kept as undocumented behavior, as it is not good practice for mappers to do that.

patrikjuvonen added a commit that referenced this issue Apr 1, 2020
* Fix #1258: Add mixed hex and RGBA parsing to XMLColorToInt

`XMLColorToInt` is used when parsing i.e. `color` attribute from map files on various elements such as vehicles, blips, radarareas, teams and so on. Before it only supported a single format for multi-value attributes.

So now it is possible to specify separate colors like so:
`#ff0, 255, 0, 0, #ff0000, #ffffff55`

Which corresponds:
yellow, red, red, transparent white

* Use unsigned int

* Handle empty strings as RGB skip
@qaisjp qaisjp modified the milestones: Backlog, 1.6 Apr 1, 2020
patrikjuvonen added a commit that referenced this issue Apr 28, 2020
…1261)"

This reverts commit a3695fb.

This fixes #1393.

This reopens #1258.

Signed-off-by: Patrik Juvonen <patrik@scope.studio>
@patrikjuvonen patrikjuvonen reopened this Apr 28, 2020
@patrikjuvonen
Copy link
Contributor Author

patrikjuvonen commented Apr 28, 2020

Fix due in next branch to be merged for 1.6 because it is not entirely backwards compatible.

Requires tweaks in race gamemode. See issue #1393.

@qaisjp
Copy link
Contributor

qaisjp commented Apr 28, 2020

According to #1393 this PR changes the behaviour of getColorFromString, which I don't believe is intentional.

getColorFromString to me, depsite the function name being vague, the wiki page sounds like it's specifically supposed to be for hex strings, rather than "any way to get a color from a string".

Are you certain that getColorFromString should be changed in this way?

@patrikjuvonen
Copy link
Contributor Author

patrikjuvonen commented Apr 28, 2020

According to #1393 this PR changes the behaviour of getColorFromString, which I don't believe is intentional.

Indeed change in behavior of getColorFromString is unintentional and went unnoticed.

As described on the Wiki page and previous behavior:

All colors used must begin with a # sign.

This no longer holds true as of the change and we need to find a solution to this.

getColorFromString to me, depsite the function name, sounds like it's specifically supposed to be for hex strings, rather than "any way to get a color from a string".

To me, FromString sounds very generic and could mean any sort of format as string, not hex specifically.

Are you certain that getColorFromString should be changed in this way?

Some possible solutions:

  1. Edit getColorFromString function to only allow hex colors starting with a # sign like before, and as already advertised on the Wiki page.

  2. Introduce backwards compatibility by adding a new optional argument to allow other supported formats, such as RGBA. By default only supports hex colors.

  3. Remove assumptive logic from ColorStringToRGBA and simply return false if the string provided does not fully satisfy the parser.

  4. Add an optional default color value argument to getColorFromString to fall back to, which - if empty, would return false if the string cannot fully satisfy the parser.

All of these fixes can be deployed in 1.5.8 instead of 1.6.

@qaisjp qaisjp modified the milestones: 1.5.8, 1.6 May 5, 2020
@botder botder modified the milestones: Next Release, Next Stage Feb 6, 2021
patrikjuvonen added a commit that referenced this issue Sep 24, 2022
@patrikjuvonen patrikjuvonen changed the title Add mixed hex and RGB(A) support to map color attributes Add hex and RGB(A) support to all map color attributes Sep 24, 2022
@patrikjuvonen patrikjuvonen removed their assignment Sep 24, 2022
@patrikjuvonen patrikjuvonen added the good first issue Good for newcomers label Sep 24, 2022
@botder botder closed this as completed in febb856 Apr 7, 2023
@patrikjuvonen patrikjuvonen removed this from the Backlog milestone Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants