You are going to be citrisized for all of these puns for sure.

Color needs some larger changes that I plan to work on later, but it does make sense for orange to be added for now.

Where are you pulling this value of Orange from?

Without having yet tested or investigated, my suspicion is that there is possibly corresponding PR's that need to be made to SpongeCommon.

Standard orange web color.

The Color class is not a catalog type and constants are not registered. There should be no conflicts or other issues caused by this at all.

Field order!

Apple cider. (note: This was requested by Zidane)

So this is what 🔶 meant...

I'd add to the puns if I could "concentrate" enough to think of one 😉

I'm defaulting to rejecting this PR outright, this whole class is a mess.

I get having our own Color interface could be handy for a few reasons, but having fields inside it that are tightly coupled with any one particular color palette (wool, dyes, sheep, fireworks, text, concrete, armor) seems beyond ridicule. (this isn't your fault it's ours, clearly)

You stated that you got it from the standard web color orange which whilst I understand when talking to you on discord is a useful color for your particle effects, doesn't really seem to belong in this class as is.

I could maybe understand pulling it if the color was related to say, dye colors, but then the dye colors class has that down pat, and you can do DyeColors.ORANGE.getColor which still doesn't match the color you want or need. Nor does ColorUtil.fromColor(DyeColors.ORANGE.getColor()) translate correctly to the EnumDyeColor in implementation, but that's another story and possibly related to 1.12 world of color update.

Yes this class needs work, but I don't think this is the right way to do it, and I don't think people want to accept this change for one person only to rip it out and break your plugin shortly afterwards.

I love the fun and puns, and I hope this causes someone to refactor the colors class sooner rather then later.

You're being mean 😢

But in all serious, this is something I've known for a while now. While I've had some fun with this PR (as have some others, it seems), the fact of the matter is that this is overlooking the bigger issues.

@Meronat and I have discussed the possibility of rebuilding the Color class and standardizing the use of colors throughout Sponge. As Ryan and I discovered, common's ColorUtils is completely broke and needs one heck of a tune-up.

I'm glad that this 'simple' PR has brought forth some deeper questions about some of the things in Sponge that we might take for granted - even something as basic as colors is not safe.

I'm more than willing to proceed with refactoring these classes and trying to update them to a standard, however I'd like to get the approval of a higher-up first as these will undoubtedly be breaking changes for plugins using the Color class (or we can keep the mis-matched names, but I'd prefer not).

In the end, it appears this has been a fruit-full endeavor after all. 🍊

@SimonFlash If you want to take this on I'm sure everyone would appreciate it. But any breaking changes would either need to be in pretty quick, or delayed until API8.

If you need to take longer I'd like to see you try and tackle the potential problems in the color utils first, and confirm whether it's due to the World Of Color update or if they have been broken since before 1.12

Also creating a test plugin would be useful, one that runs through the different color palettes, and tests serialization, as well as converting between dyecolors to implementations and back, etc.

Of course, all of this is idealistic, I'd appreciate any improvement over the current situation.

Here's another way to get Color objects that already works with the current API:

DyeColors.ORANGE.getColor()

EDIT: Never mind, already posted above.

This is going to be a lot harder than it might look.

I started making some changes to the Color class and updating the tests and in the process seem to have found some type of bug or weird result with java.awt.Colors. White, for example, is 255, 255, 255 in rgb or 0xFFFFFF in hex code (which is 16777215). This results in the following expression in the constructor:

// alpha, red, green, blue
value = ((255 & 0xFF) << 24) |
                ((255 & 0xFF) << 16) |
                ((255 & 0xFF) << 8)  |
                ((255 & 0xFF) << 0);

IntelliJ happily points out that this is a numeric overflow, and calling java.awt.Color.WHITE.getRGB() returns -1. Unless there are some really weird things going on behind the scenes that I'm not aware of, this is a huge issues as basically renders any of the java.awt.Color colors inaccurate. Please tell me I'm not loosing my sanity... at least from this.

Anyways, I'd like to get a discussion going about this with some of the core developers as it's not going to be a simple PR and will likely result in some breaking changes - and I haven't even looked at SpongeCommon yet.

All this for orange...

-1 seems like a normal result to me, considering that it's a packed int value. FFFFFFFF is how -1 is represented in unsigned hex (e.g. FFFFFFFF is -1 with 2's complement encoding).

related: #1236

One of the things I've been wondering about is how do we want to structure the static Color fields used throughout the API, and I believe this starts with the colors offered in the Color class.

The current API has a variety of colors seemingly without rhyme or reason - they match no palette I've found in my research (though similar in names to dye colors, there's an extra magenta and no orange). I updated these fields to match the 16 text colors, but I think we should standardize how we obtain and store colors for various uses.

I'd like to propose the following changes:

• Colors for color codes and dyes will be stored in TextColors and DyeColors through the #getColor method. These colors would no longer be stored in the Color class.
• TextColors should receive a #getBackgroundColor for the darker color used behind each text.
• TextColors should retrieve their colors from the net.minecraft.client.gui.FontRenderer#colorCodes array We don't have access to this class as it's only on the client, so we will need to recreate it somewhere. DyeColors already gets the color from Minecraft through MixinEnumDyeColor, but it might be a good idea to store it as a constant.
• The Color class should contain static constants for a palette of standard colors, such as either the RGB or RYB color wheel or the HTML web colors, with CSS orange of course.

Additionally, it might also be worth considering having Color cataloged by a Colors class which contains all text, dye, and common colors we use. I'm not sure what the implications of this change are, so if someone would like to weight in on that that'd be great.

Finally, a fair amount of this will be within SpongeCommon and I intended to open up a PR there for these color changes (and provide a well-needed update to our SheepFactoryColorUtil class). I'll be hoping to get to this by the end of the week.

With the PR to SpongeCommon finished (which was much faster than I expected as I didn't need to rewrite ColorUtil), this PR is complete pending feedback on the above questions and any other requested changes.

Additionally, I'd like to propose a few more notes as food for thought:

• Might we prefer using #of instead of #ofHex, #ofRgb, or other variants? Alternatively, rename the other #of methods to things like #ofVector3i for consistency.
• Have TextColor#getBackgroundColor be a default method in instead of implementing it in SpongeCommon
• Add a method for merging (dye) colors using the same formula as dying armor, shown here.
• Would also like to reiterate the note above about having a Colors CatelogType.

Whether or not the builder is the right design pattern here, it’s how we separate construction of our API elements from the implementation in a clean, non hacky manner, without resorting to performing either some hacky reflection or ASM to write portions of the class at runtime. Having static methods (such as of) that is expected to have construction code within it makes it incredibly difficult to do that. There are other places in the API where a builder isn’t quite right, but used because it’s the tool of choice and we wrap our preferred style for an API around that.

each method would do its own thing rather than stacking

Another builder that is the same can be found here. As an aside, Zidane suggested to me in my commands PR that such methods should have a set prefix, why I’ve done that up there - though it’s not done elsewhere in the API when perhaps it should have been. However, it is ultimately to aid clean separation.

That’s not to say that you can’t have static of methods, but they should become convenience methods that call the builder. So, of(hex) would call Sponge.getRegistry().createBuilder(Color.Builder.class).setHex(hex).build(), it looks clean from an API consumer’s side and it gives us the separation that we need. There is potentially an extra penalty where we need to check the state of the builder on build or when each element is called, but ultimately, I don’t see that as a problem.

I've migrated the majority of the Color class to Common and have replaced it with the new Color interface and Color.Builder builder. There are still no breaking changes caused by this PR with the Color interface directly, but there may be from moving the data builder to Common.

Discussion about the implementation should be moved to the PR in SpongeCommon.

The class to interface change being breaking at the binary level is news to me, so I withdraw my earlier statemement - this is a breaking change.

Small fix in the above commit for a requested change.

Here are the above potential projects remaining for this PR:

• Have Color be a CatelogType? (various options, +1 to NamedColor from dual)
• Remove @Deprecated methods (recompile to potential refactoring)

As API 7 is nearing a stable release, I'd like to move for this PR to be accepted by the end of the month. There are still two discussion points remaining with regards to API:

• Should we remove @Deprecated methods? While this doesn't provide developers with a lot of time to address any issues, this PR already requires a recompile and the effects of these changes are limited.
• I would also like to update the various mixColor methods, especially for dye colors. These methods aren't used in Sponge (and likely aren't used outside of Sponge), as well as being poor/inaccurate implementations.
  • Mixing RGB colors isn't an easy process, and while averaging all their components is generally close enough it isn't ideal and shouldn't be used in any type of environment. I suggest removing this from the API entirely. If someone would like this effect, they can convert the color to a Vector3i and use the methods from there (which is much more efficient for two values anyways).
  • Mixing dye colors has the same flaws as rgb colors (as the use the same method) but is much more obscure. Additionally, the formula used is different from the one used by Minecraft to calculate the result of blending dyes with armor. I suggest either the removal of these methods completely or converting them to be default methods of Color that use the right formula for mixing dyes.

Beyond that, if anyone has any remaining suggestions now's the time.

The benefit of leaving the deprecated methods, even if a plugin requires a recompile, is that you can attach documentation to them, allowing you to point developers to the new, improved methods. So, leaving them is not a bad thing for that reason.

I still feel like we should have a named colour catalogue type for the common colours, if only to bring this in line with the rest of the API design, singular class name for the type, plural for the constants.

I'm going to have to pass on having this merged for API 7. API 8 (Minecraft 1.13's compatibility update) is more likely where it will have a more fitting merge. There's more about this PR that I'd develop a bit further, as @dualspiral has mentioned, that I can't anticipate enough time to dedicate before API 7's release date.

So trying to work this down to a resolution, have we decided on making colors a CatelogType or is that still up in the air? With regards to the mixColor methods as explained above, what are your thoughts on the different options presented?

Additionally, what are the other things with this PR that you'd develop further? Adding a CatelogType is a bit of a project, but isn't something unique that needs to be developed specifically for this PR. From the sound of it there seems to be something I'm missing, so what other components are there?

I would like to see convergence on a consistent API with the broader Sponge ecosystem. My thoughts are:

• Color as a basic interface probably should not extend CatalogType.
• A new NamedColor should extend CatalogType for our... well, named colours.
• A NamedColors or Colors (though probably the former) class with static references to all current colours and any others that make sense to have.
• We might not want to allow NamedColors to be registerable. There might be a way to hide the NamedColor interface in that case, but it's not something I've looked into.

I'm not happy with the deprecation of the colours we currently have in order to favour the 16 html colours and orange. Why the 16 standards and orange? I get that this PR was originally about adding the colour orange, but we have significantly expanded the goal here and removing the traditional Minecraft colours that a lot of plugins might still rely on shouldn't be removed. Prime example - our TextColors, the ones that were originally the minecraft colour codes, are backed by some of this colours, and removing the deprecated colours would break that. We should closely match what Minecraft does with colours and all pallets, not just suggest 16+1 colours from an arbitrary standard which do not align with Minecraft.

On mixing colours - I don't know if it's used. I don't know why it's in there and who put it in - I'm not going to pass judgment on whether it should be there or not. Note that my comment on the Common PR was on the basis that it would be there.

To start with mixing colors, I completely get what you're going for there. The actual methods aren't used anywhere apart from Sponge, and as a user I don't think I'll even be in the situation where I'll want to mix colors like that as opposed to using my own with the sole exception of mixing dyes for armor (which uses a different formula anyways). This is really a gabizou call on whether to keep this or not, but I don't see any reason why it's necessary.

The colors Minecraft uses are very much all over the place, so it's near impossible to find a standard. While dyes themselves have a color, dyed blocks each have their own color schemes so it doesn't make sense to attempt to catalog those (you'd have over 100 colors and they'd have to be named WOOL_RED and such). This isn't feasible or helpful to us, so I don't think we should try documenting all colors

The only color codes that are reasonably standardized in Minecraft are the chat colors, which are almost identical to the CGA colors. The CGA colors using a particular formula based on a colors number with one exception - color 6, which is brown instead of an olive color. This is also the sole exception in Minecraft, with brown being replaced with orange instead.

If we were to use text colors, this wouldn't cover some of the main colors and are slightly off from the standard color codes. Additionally, note that these colors are technically a catalog type through TextColors as they have a getColor method (and now a getBackgroundColor method).

Prior to this PR, the Color class contained a variety of colors that didn't match up with any standard color palette - it was close to a couple, but never exact. The old class had MAGNETA, PURPLE, and DARK_MAGENTA (which matches no set in Minecraft at all) and the color code choices more closely matched the web colors as opposed to Minecrafts (such as GREY being 0x808080 instead of 0xAAAAAA (text grey) or 0x555555 (text dark grey), or DARK_CYAN being 0x008B8B (which is somewhat close to cyan, presumably) over 0x00AAAA (text cyan) and so on). Simply put, there was no rhyme or reason to the colors present, their names, and their actual color values.

I wanted to standardize this to common colors that developers might want to use, so I spent a lot of time researching the different possibilities - color wheels, the aforementioned CGA colors, and HTML web colors - before decided on a superset of the web colors and CSS orange. This covers the main colors that a user might want and, while not a 'perfect' selection it is much more complete and orderly than what was existing. I'd also support included some other colors like brown and pink, but at some point we have to draw a line. This is something can isn't too difficult to change, but I felt this was a reasonable selection for our use case.

I agree that Color should not be a CatalogType for the same reasons positions are not cataloged to a World - though technically finite, it's not what the class represents and would be unfeasible to support.

Adding in a NamedColor class to handle registered colors would work, but I personally see this as a lot of additionally boilerplate for next to no benefits, especially if new colors can't be registered (which even if allowed wouldn't have much of a significant benefit - it's not like this is a Key or Item with a lot of unique properties that needs to be tracked and retrieved frequently and potentially across multiple plugins; it's literally just a color. I see the largest benefits of the registry being the ability to document objects that don't exist within Sponge and aren't available until Minecraft begins and register it's own type and retrieve it - as we're not using either of those, I just see having a CatelogType as being unnecessary additional code to have static fields. I would also prefer NamedColors over Colors if this would be the case, as this matches the Sponge style more and Colors implies Color is the implementing CatalogType.

An alternative is to create a NamedColors class that simply contains static fields for any colors we'd like to offer. This avoids jumping through hoops with the registry while maintaining the same organization effects. Nothing else in the API is in the same situation as colors, so I don't see a reason to make it structured the same as something it doesn't represent.

rior to this PR, the Color class contained a variety of colors that didn't match up with any standard color palette - it was close to a couple, but never exact. The old class had MAGNETA, PURPLE, and DARK_MAGENTA (which matches no set in Minecraft at all) and the color code choices more closely matched the web colors as opposed to Minecrafts (such as GREY being 0x808080 instead of 0xAAAAAA (text grey) or 0x555555 (text dark grey), or DARK_CYAN being 0x008B8B (which is somewhat close to cyan, presumably) over 0x00AAAA (text cyan) and so on). Simply put, there was no rhyme or reason to the colors present, their names, and their actual color values.

All of these colors (most all of them at least) were reverse engineered based on the set of Dye colors available in Minecraft, as originally we had to design the Color class for use with DyeColors and interchangeably with leather armor color mixing (which is what the mixing brings to play). As far as representing the possible list of colors available, it is very well possible to have the possible color palettes for the variety of use cases, being DyeColors as they are represented, Text colors as they are represented, and maybe some other collection of standard color palette, such as the HTML palette.

I haven't quite built up a proper response for the rest of the PR, but my response for colors and their mixing is quite clear in my opinion.

I'm sure there was a reason for having the previous colors offered at the time, but we're moving past that now. If the goal of having mixColors was for mixing dyes with armor, then it should use the correct formula. As the text and dye color palettes are already contained within TextColor and DyeColor, there isn't a reason to replicate them elsewhere. That's why I opted to include only a standard palette of commonly used colors.

I might have missed something, but I haven't seen a response from you regarding mixing colors that presents a clear resolution, so I'd have to ask you to clarify on that.

Rather then longer paragraphs, can someone make an actionable summary (like in bullet points / check marks) so we know where this PR stands?

Here's the remaining discussion points that I am aware of:

• Depreciation of mixColor methods
• Method for dye color mixing for armor (with correct formula)
• A CatalogType for provided colors instead of static fields

My thoughts and related responses can be found above.

Additionally, there is still a point about Forge support with ColoredDataProcessor for determining what things support colors. Still waiting for feedback on this as I'm not familiar with Forge.

Latest commit depreciates the mix methods and removes the Builder#mix method in Common. I've also added in brown as a color which I apparently forgot and fixed a bug where from and reset returned DataBuilder instead of Builder.

Additionally, the following is the correct method for mixing dye colors (see net.minecraft.item.crafting.RecipiesArmorDyes#getCraftingResult for comparison).

static Color mixAsDyes(Color... colors) {
    int red = 0, green = 0, blue = 0, max = 0;
    for (Color color : colors) {
        red += color.getRed();
        green += color.getGreen();
        blue += color.getBlue();
        max += Math.max(color.getRed(), Math.max(color.getGreen(), color.getBlue()));
    }
    float mul = max / (Math.max(red, Math.max(green, blue))) / colors.length;
    return builder().rgb((int) (red * mul), (int) (green * mul), (int) (blue * mul)).build();
}

As the text and dye color palettes are already contained within TextColor and DyeColor, there isn't a reason to replicate them elsewhere

No - the backing Colors should be catalogued. Anything catalogued that uses a colour should use a catalogued named colour too. For TextColor, as you propose adding a background colour, the backing colours should be catalogued. DyeColor, on the other hand, is just a wrapper for a colour, but if we have a wrapped colour and colours should be catalogued, I'd argue that those colours should be catalogued too.

Catalogue the standard colours (that's fine), but catalogue the backing text and dye colours too. As @gabizou said, there are reasons as to why the colours were chosen. Just because it is also represented in the TextColor class (for example) doesn't mean we should hide it there.

Also, I read the response as keeping the mix colour methods. Now knowing what I know (I've never cared for mixing colours for armour and whatnot), I can see the value in the methods. Redo it to use the method you pinpointed, sure, but there are uses for mixing colours ahead of time.

I think the following needs to happen:

• Keep the mix methods on colours - at least provide a method that allows this
• Correct the mix method to use the standard method you've discovered
• Create a named colour catalogue type
  • Ensure that all standard/expected text colours and dye colours are catalouged (could prefix colours with TEXT_ or DYE_ to remove ambiguity if we really need to.)
  • Add any appropriate standard colours (maybe this could be STANDARD_RED etc.)

It might be worth waiting for gabi's input before continuing as he is ultimately the person who signs off on the API design.

The only color constants to be in the API will be the ones that exist in the base game.
This is a Minecraft API