-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
Orange you glad to see this a-peel-ling PR? #1699
Conversation
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 |
Field order! |
Apple cider. (note: This was requested by Zidane) |
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 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 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 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
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 // 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 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 |
Hex color methods and fields now use Hex over Rgb Range precondition on hex and rgb values Depreciated ofRgb, getRgb, of(Vector3f), of(Vector3d) Changed hashCode, equals, and toString implementations Updated ColorTest tests
Changed PURPLE to LIGHT_PURPLE Remove class name for static methods Re-ordered methods for consistency Added asVector3i method Added hashCode test in testEquals()
@@ -83,7 +87,7 @@ | |||
* @return The color object | |||
*/ | |||
public static Color ofHex(int hex) { | |||
checkArgument(0 <= hex && hex <= 0xFFFFFF, "hex (" + hex + ") must be in range 0 to 0xFFFFFF"); | |||
checkArgument(0 <= hex && hex <= 0xFFFFFF, "hex (%s) must be in range 0 to 0xFFFFFF", hex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you show the decimal value in the bracets, it would be a good idea to show the decimal value for 0xFFFFFF
as well.
"hex (%s) must be in range 0 to 0xFFFFFF (16777215)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point; I can also include the hex value 0xFF
with ofRgb
errors for consistency.
Would it be beneficial to include a print out of the hex value as well using Integer.toHexString(hex)
and likewise for the ofRgb
messages?
One of the things I've been wondering about is how do we want to structure the static 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:
Additionally, it might also be worth considering having 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. |
Changed static fields to web colors + orange Updated tests
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:
|
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
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 That’s not to say that you can’t have static |
Added Color and Color.Builder interfaces Converted TextColor#getBackgroundColor to default method
I've migrated the majority of the Discussion about the implementation should be moved to the PR in SpongeCommon. |
public boolean equals(Object obj) { | ||
return this == obj || obj != null && getClass() == obj.getClass() && this.rgb == ((Color) obj).rgb; | ||
} | ||
interface Builder extends ResettableBuilder<Color, Builder> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Individual red, blue, green setters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that in one attempt, but there isn't really a point to it. I'd say about 99% of use cases are going to be through the default methods, and it's both easier and faster to set them all at the same time. This also ensures that there's never a partial color in the builder, which will be 5x as important if we decide to ignore alpha values rather than removing them (which means we need to save a rgb value as well that would need to be updated for each of those method calls). It's an overhead that I don't think is worth it in the slightest even though it's a bit against the builder mentality - but so is the entire class anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remember (whoever is eventually thinking of potentially merging this) that changing a class to an interface is a breaking change in that it represents a binary breakage with any compiled plugin calling any method on the type at all.
@@ -50,7 +50,9 @@ | |||
* | |||
* @return the RGB background color of this text color | |||
*/ | |||
Color getBackgroundColor(); | |||
default Color getBackgroundColor() { | |||
return Color.of(getColor().toVector3i().div(4)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't default implement this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it's basic, but it is a valid property that every TextColor
has. Are you suggesting to revert it to a interface method or remove it entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just make it a regular interface method, the only things that should be default implemented is trivial calls to overloaded methods.
public Builder() { | ||
super(Color.class, 1); | ||
} | ||
Builder mix(Color... colors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better pattern here is that mix takes one color and mixes with the current color of the builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would have default implementations mix with the color black. Any static method for mixing would need to pull out the first element of the array, build thay, then copy the remaining elements to a new array to be used with this method. The mix methods aren't used in Sponge (bar tests) and are probably used in few to no plugins. I'd be more in favor of removing them entirely.
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:
|
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:
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 Additionally, what are the other things with this PR that you'd develop further? Adding a |
I would like to see convergence on a consistent API with the broader Sponge ecosystem. My thoughts are:
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 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 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 Prior to this PR, the 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 Adding in a An alternative is to create a |
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 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:
My thoughts and related responses can be found above. Additionally, there is still a point about Forge support with |
Depreciated mixColor methods and documented their formula Override from and reset to return Builder instead of DataBuilder
Latest commit depreciates the mix methods and removes the Additionally, the following is the correct method for mixing dye colors (see 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();
} |
No - the backing 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 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:
It might be worth waiting for gabi's input before continuing as he is ultimately the person who signs off on the API design. |
|
SpongeAPI | SpongeCommon
For just
onetwo PR's to Sponge, you too can support orange and show him some love!All puns are 100% oranginal and totally weren't stolen from google. Or Parker.