Fix colors

[ernie]

Alternative fix to #33 that works across both games without much in the way of code changes. Tweaked the positioning of the selected color indicator in team arena while I was at it, because it looked wonky.

Demo video: https://youtube.com/shorts/N9rZJ-VkIu8

[RippeR37]

@ernie

I assume current master has wrong order/not-matching colors for TA, right?

I wonder why do we need this instead of just reverting the logic to what ioq3 has for these things? I'm somewhat afraid that by diverging further from ioq3 we might find ourselves with more issues around colors not matching (e.g. when rendering some text or player names/tags etc.), so overall the question would be why not to revert this logic completely to what ioq3 had for colors etc. instead of still pushing the baseq3a implementation (based on CG_ColorFromChar which does differ in results from CG_ColorFromString)?

The idea to put fixed fx_base.tga to Q3VR PAK seems good, should make users of this port safe from the issue. 👍

[ernie]

Yes, this is just creating consistency across the board. Have tested across both Q3A and TA and all work well. Since we don't load QVM files, there is no risk here in seeing different behaviors on different servers and overall the baseq3a approach is cleaner.

[ernie]

Text and other rendering also behave properly

[ernie]

Obviously you're the maintainer, here, but this is a mild divergence from ioq3 and in an area of code that isn't frequently touched, so seems low-risk and better to have one and only one path for color than a separate one in the case of the railgun color, to me.

[RippeR37]

Obviously you're the maintainer, here, but this is a mild divergence from ioq3 and in an area of code that isn't frequently touched, so seems low-risk and better to have one and only one path for color than a separate one in the case of the railgun color, to me.

Ack. For what it's worth I was considering reverting to the "only way" that was ioq3-based prior to baseq3a changes being ported and keeping that version, as I'm unsure if this new version (from baseq3a) is actually needed for anything or just a different implementation for the same functionality (in which case I'd prefer to stick with ioq3 implementation as q3vr is based on it and it would make it easier to rebase on top of newer version later on). But yeah, right now it might not be even easy to get back to original implementation for this even if the assumption (that this isn't required for any new feature from bq3a) would be true (which I'm not sure either).

I'll do a bit of testing and most likely merge this, but for future work I think the goal should be to only merge changes which actually improve UX or introduce new (ideally optional) feature/functionality/... If there is a code change just for the sake of doing same thing in a different way then I think we should keep the implementation close to ioq3 version.

[ernie]

Ack. If the other function can solve for all the use cases of the new one, I wouldn't be opposed to swapping them around. I added this back when adding support for enemyColor and teamColor, etc. I can take another pass and see if it can be flipped back around, if you like.

[RippeR37]

Ack. If the other function can solve for all the use cases of the new one, I wouldn't be opposed to swapping them around. I added this back when adding support for enemyColor and teamColor, etc. I can take another pass and see if it can be flipped back around, if you like.

No, I think its fine, if this version works and solve all our problems I think its good for now and we can revisit this topic in the future if it becomes a problem of any kind. For now its probably not worth spending more time if everything works fine. :)

[RippeR37]

Okay, I tested it and it looks like it overall works (keeps color in settings aligned with what's on the screen). The issue is that with this implementation meaning for cvars color1 and color2 are changed (due to different impl we discussed). On ioq3 and without this PR, the color1 and color2 cvars default to 4 and 5 and this means red trail effect, and with this patch it means blue. It also means that when someone starts a game with this patch for the first time default color is blue instead of intended/red.

I think if there are issues with Q3TA then they need to be fixed in a way that it makes it compatible with ioq3 otherwise (even though its not that big of a deal) people might get weird behavior or their configs might not match what they want.

[RippeR37]

Okay, LGTM.

I did a small manual edit to the provided fx_base.tga image to add a black border to it and a small inner-shadow effect to make it look a bit nicer than just plain color (it now also looks like the slider is smaller than the current selector, as it was before). Merged manually to master.

Thank you! :)