Species coloring refactor #40067
Species coloring refactor #40067
Conversation
PJBot
added
S: Untriaged
|
TODO:
|
TheShuEd
added
the
S: DO NOT MERGE
|
Alternative PR to #39175 I think? |
|
oh, real |
|
Massive life saver for a race like this one: Out of scope, but if you can also add a single-color hair support (or same limits per hair) you are a life saver. |
|
Directly impacts Vulps development here, as we're wanting to avoid zany fur colours. |
|
If this isn't done yet can you kick it to Draft? Makes it easier to see what's WIP. |
|
I just wanna also check integration tests in github, so |
FairlySadPanda
added
P2: Raised
|
I've triaged this PR. I've removed DO NOT MERGE as it's redundant for Draft PRs :)
Understandable fam |
| coloringRules: | ||
| - !type:Hues |
There was a problem hiding this comment.
This rules langugage is not expressive enough. I can guess that this means "this species has restrictions on the hues it uses" but it should be phrased as "UseRestrictedHues" or similar, so someone reading the YAML knows intent without having to be familiar with the rules engine.
|
|
||
| namespace Content.Shared.Humanoid.ColoringScheme; | ||
|
|
||
| public sealed partial class HumanToned : ColoringSchemeRule |
There was a problem hiding this comment.
| public sealed partial class HumanToned : ColoringSchemeRule | |
| public sealed partial class UsesHumanSkinTonesRule : ColoringSchemeRule |
|
|
||
| namespace Content.Shared.Humanoid.ColoringScheme; | ||
|
|
||
| public sealed partial class Hues : ColoringSchemeRule |
There was a problem hiding this comment.
At the moment this is probably
| public sealed partial class Hues : ColoringSchemeRule | |
| public sealed partial class ClampHuesToMinimumLightnessRule : ColoringSchemeRule |
Note that for colors, perceived brightness is not equal across the range. A TODO here noting this for future improvement would be a good idea.
|
|
||
| namespace Content.Shared.Humanoid.ColoringScheme; | ||
|
|
||
| public sealed partial class TintedHues : ColoringSchemeRule |
There was a problem hiding this comment.
| public sealed partial class TintedHues : ColoringSchemeRule | |
| public sealed partial class ClampHuesToMinimumSaturationRule : ColoringSchemeRule |
This is also covering minimum lightness, so do you need it? Can a speceies just use both the Hues and TintedHues rules?
|
|
||
| namespace Content.Shared.Humanoid.ColoringScheme; | ||
|
|
||
| public sealed partial class VoxFeathers : ColoringSchemeRule |
There was a problem hiding this comment.
Slight code smell for this to be phrased as a particular species, but Human already exists so I guess it's fine.
| public sealed partial class VoxFeathers : ColoringSchemeRule | |
| public sealed partial class UseVoxFeatherHuesRule : ColoringSchemeRule |
About the PR
The logic behind race color selection restrictions is undergoing a major refactor aimed at improving code extensibility.
Instead of an enum with different color options, which performed different logic in five places in the code using switch case, all the logic is now elegantly placed in a single class inherited from
ColoringSchemeRule. This class contains all the logic related to color validation.Inside
SpeciesPrototype,SkinColorationhas been removed and replaced withList<ColoringSchemeRule> ColoringRules.Why / Balance
Adding new types of species color limiters is simply impossible—you have to go through a dozen files, making changes in a bunch of different places. For forks, this is especially awful.
Media
no media
Requirements
Breaking changes
HumanoidSkinColor SkinColorationfield insideSpeciesPrototypewas replaced withList<ColoringSchemeRule> ColoringRules.Changelog
no cl