Update SpongeCommon Color Usage #1625

SimonFlash · 2017-11-28T23:29:20Z

This PR updates SpongeCommon for the changes made in SpongeAPI, notably the deprecation of Color#ofRbg(int) and the addition of TextColor#getBackgroundColor(). Additionally, this also removes the ColorUtil class and migrates any methods still used to ColoredDataProcessor or NBTDataUtil.

Edit: I have no idea what I did the first time around, but thanks to Faithcaio and phit things are no longer a mess. In the meantime, I have been tasked with reading a git book - adiou!

Removed ColorUtil and moved necessary methods into ColoredDataProcessor/NBTDataUtil Added background color to SpongeTextColor (cherry picked from commit 4c22731)

Added a builder system for creating colors Updated code for current API The rgb value now strip any alpha value Red, green, and blue values now use bitwise and to ensure values are in range 0-255 Migrated ColorTest to Common (needs to be updated to work with the builder registry)

SimonFlash · 2017-12-01T03:39:27Z

The trend throughout most of Common is to have a separate ...Builder class, but this seems to apply to classes such as ItemStack that aren't implemented in Sponge. If we would prefer have a SpongeColor and SpongeColorBuilder it's an easy change to make.

Also, I'm not sure how to setup the ColorTest to work in Common because the registry would need to be initialized to get the color builder. If anyone could provide direction on this that'd be great.

dualspiral · 2017-12-01T14:05:20Z

Also, I'm not sure how to setup the ColorTest to work in Common because the registry would need to be initialized to get the color builder. If anyone could provide direction on this that'd be great.

Annotate your test class with @RunWith(LaunchWrapperTestRunner.class). That'll run by initialising a dummy server first, letting you test using the registry.

gabizou

If you're going to want this tested, you at the very least want to have a test plugin that will be able to provide some colored items, including dyed leather armor pieces, etc. to validate that the color changes are working (and of course, translation of colors between them).

Fixed ColorTest (junit) to work with the registry Added ColorTest (plugin) into test plugins

SimonFlash · 2017-12-03T07:08:21Z

Update for the TextColor change in API, a fix for ColorTest (junit) to work with the registry, and the addition of a ColorTest plugin for proof-of-concept (and rainbows). I've tested this with SpongeForge compiled from a couple hours ago without any issues. Colors are being retrieved properly from items and/or dye colors and can be offered back to the item with Keys.COLOR successfully.

Remaining potential projects in this PR:

Have #mix use the Minecraft formula for mixing dyes
Remove #mix completely (uses are few and far between; not used in Sponge. Is breaking)
Implementation changes from projects in API

phit · 2017-12-03T07:23:38Z

make the testplugin something that's not enabled on every click, preferably something like set color for item in hand when running a command and get color from item in hand

ryantheleach · 2017-12-04T01:36:14Z

make the testplugin something that's not enabled on every click,

This is good advice, but considering the growing number of test plugins I'm considering making a basic framework for dealing with this after I've cleared the rest of my backlog.

gabizou · 2017-12-05T07:23:14Z

testplugins/src/main/java/org/spongepowered/test/ColorTest.java

+
+    @Listener
+    public void onPrimaryClick(InteractItemEvent.Primary event, @First Player player) {
+        player.getItemInHand(HandTypes.MAIN_HAND).ifPresent(item -> {


If this is a test, please make it toggle able either by command or configuration option. By no means should this always emit some message randomly causing people to freak out about debug messages when working on implementation ;).

Tell that to whoever set up the inventory tests, I still get "You clicked on slot 7" messages every time I go to pull out an item xD. I'll switch those over to commands that can be used.

I'm not going to respond to the remaining reviews individually, but I'll address them in my edits tonight. I think I had a bit to much fun with rainbows (though they are fun) and let some of the good practices slip away from me. @ryantheleach, could you supply some info on what you'd like to see as a standard for test plugins so I can do that now and not have to rebuild it later?

You won't have to rebuild it later, just make it somewhat toggleable to pass review, this is something larger that I havn't fully fleshed out yet.

gabizou · 2017-12-05T07:23:38Z

testplugins/src/main/java/org/spongepowered/test/ColorTest.java

+        });
+    }
+
+    private static double radians;


For the sake of style, this should be on the top of the class.

gabizou · 2017-12-05T07:24:50Z

testplugins/src/main/java/org/spongepowered/test/ColorTest.java

+    }
+
+    private static double radians;
+    private static final double increment = Math.PI / 7, shift = 2 * Math.PI / 3;


Should be formatted as a constant (

private static final double INCREMENT = Math.PI / 7; private static final double SHIFT = 2 * Math.PI / 3;

gabizou · 2017-12-05T07:25:15Z

testplugins/src/main/java/org/spongepowered/test/ColorTest.java

+
+    private static double radians;
+    private static final double increment = Math.PI / 7, shift = 2 * Math.PI / 3;
+    private static final Vector3d shifts = Vector3d.from(2 * shift, shift, 0), offset = Vector3d.from(127.5);


And here, don't tail on fields like this. It's bad legibility for others.

gabizou · 2017-12-05T07:25:58Z

src/main/java/org/spongepowered/common/data/processor/data/item/ColoredDataProcessor.java

+        super(ColoredDataProcessor::supportsColor, Keys.COLOR);
+    }
+
+    private static boolean supportsColor(ItemStack stack) {


Any chance this can be overridden in Forge for compatibility with other mods?

Unsure, as my experience with both Forge and the Data API is lacking. This method itself was just copied from ColorUtil so I really don't know the specifics of it and what would need to be done to support this.

gabizou · 2017-12-05T07:27:30Z

src/main/java/org/spongepowered/common/registry/type/text/TextColorRegistryModule.java

+        addTextColor(TextFormatting.DARK_AQUA, Color.of(0x00AAAA));
+        addTextColor(TextFormatting.DARK_RED, Color.of(0xAA0000));
+        addTextColor(TextFormatting.DARK_PURPLE, Color.of(0xAA00AA));
+        addTextColor(TextFormatting.GOLD, Color.of(0xFFAA00));


The more I look at this, the more I prefer the ofRgb instead of just flag of, since you can define how it's formatted.

I'm fine with that; though in my mind of is more consistent with the naming of other static methods used throughout Sponge. If we do revert this change, then I would like to be consistent and use ofVector3i, ofJavaColor, and so on.

ofVector3i should also be ofRgb as its simply an overload to take a vector.

Fair enough, but I don't see the reasoning to have ofRgb over of unless Color is designed to support multiple color formats, which is unneeded as well as more complex. We represent colors using RGB, so ofRgb doesn't provide any additionally information and, as mentioned, is less consistent with the rest of Sponge.

This change in particular, though, is more specific to the API over Common.

Its ensuring that we don't back ourselves into a confusion corner later if say we wanted to add an ofHSV (not a bad idea as it is a much easier color space to work with for many use cases).

An ofHSV function which is a simple conversion into the RGB color space would be simple and non-breaking. No need to change the backing storage, or even consider that right now. We just don't want a plain of function that doesn't even make clear in the simplest case between ofRGB and ofBGR.

Converting HSV to RGB is not something I consider worthwhile (see this question that has the conversion formulas). For reference, the java.awt.Color class is defined to represent colors in sRGB format (link from the class javadocs). If a developer would prefer to use HSV, they can use the conversion methods in java.awt.Color.

Color.of(int rgb) provides the same specification that it is in RGB format as does new java.awt.Color(int rgb). I maintain that the Color class should be defined to use the RGB format and use #of, as #ofRgb is unnecessarily repetitive and is different from the #of standard used elsewhere in Sponge.

I think I’ve said this before, the method should very well be named ofRgb to avoid confusion in the format the colors are presented. Java API is not a standard to hold us up to on making such a method name decision. I’m putting my decision as it was mentioned before.

Here's what I'm tracking so far. What are we doing with the one for java.awt.Color?

#of(int rgb) -> #ofRgb(int rgb)
#of(int red, int green, int blue) -> #ofRgb(int red, int green, int blue)
#of(Vector3i vector3i) -> #ofRgb(Vector3i vector3i)
#of(java.awt.Color) -> ??? (#ofJavaColor?)

Can make it ofColor since the class type is implied by the argument type. Otherwise fine.

gabizou · 2017-12-05T07:29:04Z

src/main/java/org/spongepowered/common/util/SpongeColor.java

+
+        @Override
+        public Builder reset() {
+            return rgb(0, 0, 0);


A little wasteful when all you need to do is just set the fields and return this...

That's true, just seemed simpler to me to do it like that. It might also be advantageous to us to use -1 as the default value, that way we know a color has been set.

gabizou · 2017-12-05T07:29:29Z

src/main/java/org/spongepowered/common/util/SpongeColor.java

+
+    }
+
+    public static final class DataBuilder extends AbstractDataBuilder<Color> {


To be quite honest, this should be merged with the Builder, and not as a separate class.

Can this be done by having Builder extend AbstractDataBuilder<Color> and implement Color.Builder, or is there more to it? Is there any example of this combination used elsewhere in Common that I can reference?

Yes, you can do that. Example as requested.

Perfect, thank you.

Have values be -1 by default to check for undefined colors

SimonFlash · 2017-12-21T21:49:14Z

With API 7 receiving a stable release at the end of the month, I'd like to push for this PR to be merged prior to it's release. Please see the API PR for topics more related to the API.

In Common, there is really only one outstanding discussion point - overriding ColoredDataProcessor#supportsColor within SpongeForge for mods. I don't know what would be required to handle this, but if this is something that needs to be considered I could definitely use assistance from someone familiar with the Forge side of things.

dualspiral · 2017-12-22T09:50:23Z

src/main/java/org/spongepowered/common/util/SpongeColor.java

+        }
+
+        @Override
+        public Builder mix(Color... colors) {


I'm shaky about this behaviour. A builder shouldn't do any processing like this pre-build.

My feeling is that we should offer a mix(Color) method that we can chain up (and this varargs method would default to chaining these up), and that we store the colours that we want to mix. Then, when build() is called, that's when the mixing occurs. This means that we can use the builder pattern for both mixing colours and specifiying a colour from RGB values, or both.

It allows us to offer a Color.mix(Color...) helper method that just calls Color.builder().mix(...).build() too.

To emulate the current behaviour, you could then just create the colour in a separate builder, so Color.build().rgb(...).mix(Color.mix(...)).build()

I don't think I'm understanding what you mean by this. I believe you're suggesting that each call to mix should save any passed in colors to a list, which would be mixed internally when build is called.

I'm still a proponent of deprecated the mix methods entirely as per my reasoning in the API pull. One of the things for me is that the Color class, in my opinion, should not be represented by a builder - it's only like that so the implementation can exist in Common and use the builder registry in API. The methods don't interact in the same way as most builders as ever method essentially overrides another.

With the change to reset to have the default values be -1, the way this method should be functioning is to mix with an existing color in the builder if defined. I believe what you're getting at is that chaining the methods together should have the same effect as combining them all together into one call, which they currently do not.

However, I would still prefer deprecating these methods for removal as they are impractical.

ImMorpheus · 2019-02-08T22:17:02Z

SpongePowered/SpongeAPI#1699 (comment)

SimonFlash requested review from Deamon5550, Faithcaio, gabizou, kashike and stephan-gh as code owners November 28, 2017 23:29

Updated use of Color#ofRbg(int) to Color#ofHex(int)

09d0d5b

Removed ColorUtil and moved necessary methods into ColoredDataProcessor/NBTDataUtil Added background color to SpongeTextColor (cherry picked from commit 4c22731)

SimonFlash force-pushed the feature/orange branch from 4c22731 to 09d0d5b Compare November 28, 2017 23:40

phit removed request for gabizou, Deamon5550, Faithcaio, stephan-gh and kashike November 28, 2017 23:42

SimonFlash mentioned this pull request Nov 29, 2017

Orange you glad to see this a-peel-ling PR? SpongePowered/SpongeAPI#1699

Closed

gabizou requested changes Dec 1, 2017

View reviewed changes

Reverted TextColor#getBackgroundColor to implementation

c800143

Fixed ColorTest (junit) to work with the registry Added ColorTest (plugin) into test plugins

gabizou requested changes Dec 5, 2017

View reviewed changes

SimonFlash added 3 commits December 6, 2017 02:31

Update ColorTest test plugin to be somewhat less of a mess.

884a56c

Combine DataBuilder with Builder

b1a0c5f

Have values be -1 by default to check for undefined colors

Rename default methods in Color.

40b1140

dualspiral reviewed Dec 22, 2017

View reviewed changes

gabizou added this to the API 8.0 milestone Dec 22, 2017

gabizou added version: 1.16 (u) API: 8 api: pending the API is not finished (input wanted, etc.) labels Dec 22, 2017

Removed mix method

00599cb

ImMorpheus closed this Feb 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SpongeCommon Color Usage #1625

Update SpongeCommon Color Usage #1625

SimonFlash commented Nov 28, 2017 •

edited

SimonFlash commented Dec 1, 2017

dualspiral commented Dec 1, 2017

gabizou left a comment

SimonFlash commented Dec 3, 2017

phit commented Dec 3, 2017

ryantheleach commented Dec 4, 2017

gabizou Dec 5, 2017

SimonFlash Dec 6, 2017

ryantheleach Dec 6, 2017

gabizou Dec 5, 2017

gabizou Dec 5, 2017

gabizou Dec 5, 2017

gabizou Dec 5, 2017

SimonFlash Dec 6, 2017

gabizou Dec 5, 2017

SimonFlash Dec 6, 2017 •

edited

Deamon5550 Dec 6, 2017

SimonFlash Dec 6, 2017

Deamon5550 Dec 6, 2017

Deamon5550 Dec 7, 2017

SimonFlash Dec 8, 2017 •

edited

gabizou Dec 8, 2017

SimonFlash Dec 8, 2017

gabizou Dec 8, 2017

gabizou Dec 5, 2017

SimonFlash Dec 6, 2017

gabizou Dec 5, 2017

SimonFlash Dec 6, 2017

dualspiral Dec 7, 2017

SimonFlash Dec 7, 2017

SimonFlash commented Dec 21, 2017

dualspiral Dec 22, 2017

SimonFlash Dec 22, 2017

ImMorpheus commented Feb 8, 2019


		}

		public static final class DataBuilder extends AbstractDataBuilder<Color> {

Update SpongeCommon Color Usage #1625

Update SpongeCommon Color Usage #1625

Conversation

SimonFlash commented Nov 28, 2017 • edited

SimonFlash commented Dec 1, 2017

dualspiral commented Dec 1, 2017

gabizou left a comment

Choose a reason for hiding this comment

SimonFlash commented Dec 3, 2017

phit commented Dec 3, 2017

ryantheleach commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonFlash Dec 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonFlash Dec 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonFlash commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ImMorpheus commented Feb 8, 2019

SimonFlash commented Nov 28, 2017 •

edited

SimonFlash Dec 6, 2017 •

edited

SimonFlash Dec 8, 2017 •

edited