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

Resurrect old color picker #20742

Merged
merged 2 commits into from
Apr 8, 2023
Merged

Resurrect old color picker #20742

merged 2 commits into from
Apr 8, 2023

Conversation

PunkPun
Copy link
Member

@PunkPun PunkPun commented Mar 12, 2023

Closes #20415

Credit to @darkademic. I just added a few of the missing bits such as the update rule.

Color picker in this PR
Screenshot 2023-03-12 at 20 37 22

Old color picker
Screenshot 2023-02-27 at 22 45 54

Bleed color picker
Screenshot 2023-02-27 at 22 45 40

As you can see the the color picker looks better with S and V axes flipped.

I also implemented IColorPickerManagerInfo interface so that third party mods would be able to implement their own color pickers. When reviewing the second commit I recommend to start with the interface.

@anvilvapre
Copy link
Contributor

i wanted to mention that players in general don't mind if multiple players - in the same team or not - have about the same color - for as long as they are able to recognize and distinguise their own units. so if i.e. the progress bar for your own units would have a different color than of other players, or there is some mode/key that tells you to turn other players to another tint, that could work.

the issue is not that colors may look alike, but that you don't know which units are yours or not.

OpenRA.Mods.Common/Traits/World/ColorPickerManager.cs Outdated Show resolved Hide resolved
mods/cnc/rules/world.yaml Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/ColorPickerManager.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/TraitsInterfaces.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/TraitsInterfaces.cs Outdated Show resolved Hide resolved
@PunkPun
Copy link
Member Author

PunkPun commented Mar 13, 2023

Made the IColorPickerManagerInfo interface much more generic and cleaned up a bunch of code in the process

@Mailaender
Copy link
Member

Changing to a predefined color and then switching back to mixer does not update properly:

image

@PunkPun
Copy link
Member Author

PunkPun commented Mar 16, 2023

fixed

Mailaender
Mailaender previously approved these changes Mar 16, 2023
@Mailaender
Copy link
Member

There is a superflous dot in the first commit message.

@PunkPun
Copy link
Member Author

PunkPun commented Mar 18, 2023

If you so wish @abcdefg30

Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

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

We still seem to be missing some colors very close to white, but I don't think that was ever the goal. We do now have so very dark colors.
Bonus points for yaml hex colors actually matching their in-game representations (making that particular ORAIDE feature useful again). 👍

Couldn't spot issues ingame. I went over the first commit and got halfway through the second, so this isn't a complete review, but I still have a few comments to work on.

[Desc("List of saturation components for the preset colors in the palette tab. Each entry must have a corresponding PresetHues definition.")]
public readonly float[] PresetSaturations = Array.Empty<float>();
[Desc("List of colors to be displayed in the palette tab.")]
public readonly Color[] PresetColors = Array.Empty<Color>();
Copy link
Member

Choose a reason for hiding this comment

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

Should we be checking when loading these colors if they violate the Saturation or ValueRanges above?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are going to get fixed anyway. But we could add a lint check if you feel it's needed.

Copy link
Member

Choose a reason for hiding this comment

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

Lint checks are always nice, but I wouldn't say it's required here. I was talking about runtime and as long as these colors are fixed to valid ranges that should be fine.

OpenRA.Mods.Common/Traits/World/ColorPickerManager.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/ColorPickerManager.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/TraitsInterfaces.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/ColorPickerManager.cs Outdated Show resolved Hide resolved
@penev92
Copy link
Member

penev92 commented Apr 8, 2023

Sounds like this should finally close #20415.

@penev92 penev92 merged commit d838d08 into OpenRA:bleed Apr 8, 2023
@penev92
Copy link
Member

penev92 commented Apr 8, 2023

Changelog

Note: PR NOT cherry-picked to prep-2211 pending decision on how to proceed.

@Mailaender
Copy link
Member

I vote for picking to prep-2211.

@PunkPun PunkPun deleted the colorpicker3 branch April 9, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with the new colorpicker
7 participants