FlxRandom: Fixed getObject(), removed colorExt() #1158

steverichey · 2014-06-06T19:28:55Z

Due to an error I made when refactoring FlxRandom, getObject() would always return the 0th element of the array unless you specified an end index. The StartIndex and EndIndex parameters are now nullable integers, and if left unspecified, will be 0 and Object.length - 1, respectively.

Also, I had been frustrated with the complexity of color() and colorExt(), and finally implemented a single function (now just color()) which accepts a single ColorOptions object with all the settings previously individually set by each parameter.

Gama11 · 2014-06-06T19:32:47Z

flixel/math/FlxRandom.hx

@@ -217,10 +217,21 @@ class FlxRandom
 				WeightsArray = [for (i in 0...Objects.length) 1];
 			}

+			if (StartIndex == null)


Wait.. why does this need to be nullable in the first place then? Why not just add a default value = 0?

Good catch, fixed.

Gama11 · 2014-06-06T19:33:59Z

flixel/math/FlxRandom.hx

-	 * @param   GreyScale   Whether or not to create a color that is strictly a shade of grey. False by default.
-	 * @return  A color value in hex ARGB format.
+	 * @param	Options		An object containing key/value pairs of the following optional parameters:
+	 * 						min			A minimum integer value for red, green, and blue channels. Will be 0 if unused.


Minor thing, but: it's preferable to use spaces for formatting like here, as you can see, it can lead to very inconsistent results if tabs are used.

Gama11 · 2014-06-06T19:35:47Z

flixel/math/FlxRandom.hx

@@ -425,4 +448,19 @@ class FlxRandom
 		return _recordingSeed;
 	}
 	#end
+}
+
+typedef ColorOptions = {


Not sure if @gamedevsam is gonna like this, typedefs are fairly expensive on native. ;)

Is there a better option? I just modeled this off of TweenOptions

I guess it might not really be an issue. This gives you some additional flexibility because you can store the typedef in a variable for several calls to color(). The same would be true for a class, but that wouldn't be as as self-documenting and there's a set order in which you have to set the params if you put them into the constructor, so I think I prefer a typedef here...

Gama11 · 2014-06-06T19:43:13Z

flixel/math/FlxRandom.hx

+	?min:Null<Int>,
+	?max:Null<Int>,
+	?alpha:Null<Int>,
+	?greyscale:Bool,


Any basic type with a ? will automatically be turned into a Null<T> (easy to check with $type()). So marking these all as Null<Int> (instead of Int) doesn't really do anything. I don't really mind, but it's a bit inconsistent with leaving this as a Bool.

Removed! Thanks for pointing it out.

JoeCreates · 2014-06-07T15:10:48Z

This is an improvement, though I have some further thoughts about FlxRandom.color().

I have another idea for how color might work. It would require fewer parameters but also give greater flexibility. The idea is to have just three parameters, color1, color2, and colorModel. The color model would use an enum with RGB; HSB; HSL; CMYK;. The generated color would use the respective color properties of the two provided colors and randomly interpolate for each property. The alpha channel would also be randomly interpolated between the alpha channels of the provided colors.

This would allow you to do everything you can already do, but also allow you to randomize on properties such as hue only, or saturation only.

Doing something in greyscale would simply be a case of using HSB or HSL and with hue and saturation values of 0, but now you would also have the option of randomizing the brightness with a constant hue/saturation.

On the other hand, I'm wondering if FlxRandom.color() is even that useful, though, considering randomizing each property is pretty simple in itself anyway.

Below are some comparisons of three different methods of generating a random color. Of each example the first line is not using FlxRandom.color() at all. The second line is using Steve's improved options method. The third line is interpolating between colors given a colorModel (which defaults to RGB).

// --- Example 1: Random color, no constraints (probably not often useful) ---
FlxColor.fromRGB(FlxRandom.int(0, 255), FlxRandom.int(0, 255), FlxRandom.int(0, 255));
FlxRandom.color();
FlxRandom.color();

// --- Example 2: RGB color with different properties for each ---
FlxColor.fromRGB(FlxRandom.int(100, 120), FlxRandom.int(110, 130), FlxRandom.int(115, 140));
FlxRandom.color({minRed: 100, maxRed: 120, minGreen: 110, maxGreen: 130, minBlue: 115, maxBlue: 140});
FlxRandom.color(FlxColor.fromRGB(100, 110, 115), FlxColor.fromRGB(120, 130, 140));

// --- Example 3: RGBA color with different properties ---
FlxColor.fromRGB(FlxRandom.int(100, 120), FlxRandom.int(110, 130), FlxRandom.int(115, 140), FlxRandom.int(100, 150);
FlxRandom.color({minRed: 100, maxRed: 120, minGreen: 110, maxGreen: 130, minBlue: 115, maxBlue: 140, alphaMin: 100, alphaMax: 150});
FlxRandom.color(FlxColor.fromRGB(100, 110, 115, 100), FlxColor.fromRGB(120, 130, 140, 150));

// --- Example 4: RGB color with constant range (not sure if this is actually very useful) ---
FlxColor.fromRBG(FlxRandom(100, 150), FlxRandom(100, 150), FlxRandom(100, 150));
FlxRandom.color({min: 100, max: 150});
FlxRandom.color(FlxColor.fromRGB(100, 100, 100), FlxColor.fromRGB(150, 150, 150));

// --- Example 5: Random red only ---
FlxColor.fromRGB(FlxRandom.int(110, 150), 100, 100);
FlxRandom.color({minRed: 110, maxRed: 150, min: 100, max: 100});
FlxRandom.color(FlxColor.fromRGB(110, 100, 100), FlxColor.fromRGB(150, 100, 100));

// --- Example 6: Ranged greyscale color ---
FlxColor.fromHSB(0, 0, FlxRandom.float(0.5, 1));
FlxRandom.color({greyscale: true, min: 128, max: 255});
FlxRandom.color(FlxColor.fromHSB(0, 0, 0.5), FlxColor.fromHSB(0, 0, 1), ColorModel.HSB);

// --- Example 7: Random hue and brightness, constant saturation ---
FlxColor.fromHSB(FlxRandom.float(100, 360), 1, FlxRandom.float(0, 1));
// Not possible with properties object method!!!
FlxRandom.color(FlxColor.fromHSB(100, 1, 0), FlxColor.fromHSB(360, 1, 1), ColorModel.HSB);

// --- Example 8: Random hue only ---
FlxColor.fromHSB(FlxRandom.float(100, 360), 1, 1);
// Not possible with properties object method!!!
FlxRandom.color(FlxColor.fromHSB(100, 1, 1), FlxColor.fromHSB(360, 1, 1), ColorModel.HSB);

In the above:-

In 4/8 cases, it is quickest not to use FlxRandom.color() at all
In 2/8 cases, the ColorModel method is only slightly better than no FlxRandom.color()
In 2/8 cases, the existing method is quickest, but these arguably give the least useful results.
in 2/8 cases, the existing method cannot be used at all!

Note also that the first and third methods tend to be shorter than the existing FlxRandom.color() method, in spite of the presence of several extra "Flx" prefixes.

In summary, I actually think FlxRandom.color() is a bit pointless, in any implementation. I could only think of two cases where it provided any significant help, but neither of these cases produce very useful results. Perhaps it is worth having for the cases where no constraints are needed (i.e. a FlxRandom.color() with no parameters). As soon as you wish to add some constraints, though, chances are you're better off not using it.

Of course, having something that is mostly redundant isn't that bad. Perhaps it's worth considering this redundancy when it comes to removing or improving it (and thus making a breaking change), though. There's possibly not much point in improving FlxRandom.color() if FlxRandom.color() isn't so useful to begin with.

steverichey · 2014-06-07T16:09:35Z

I agree that color() is probably not the most useful FlxRandom
function, but I think the proposed implementation is simple and practical
enough for most purposes, but I see how your method could offer more
flexibility.

Personally, I use color() quite a bit for prototyping, which is why I
kept it for this revision. If you'd like to change it, submit a pull
request! Or we could pull in the rest of the contributors if this is an
issue you think should be opened up for further discussion.
On Jun 7, 2014 10:10 AM, "Joe Williamson" [email protected] wrote:

This is a good idea. I have another idea for how color might be improved,
though. It would require fewer parameters but also give greater
flexibility. The idea is to have just three parameters, color1, color2,
and colorModel. The color model would use an enum with RGB; HSB; HSL;
CMYK;. The generated color would use the respective color properties of
the two provided colors and randomly interpolate for each property. The
alpha channel would also be randomly interpolated between the alpha
channels of the provided colors.

This would allow you to do everything you can already do, but also allow
you to randomize on properties such as hue only, or saturation only.

Doing something in greyscale would simply be a case of using HSB or HSL
and with hue and saturation values of 0, but now you would also have the
option of randomizing the brightness with a constant hue/saturation.

I am partly wondering if FlxRandom.color() is even that useful, though,
considering randomizing each property is pretty simple in itself anyway.

Below are some comparisons of three different methods of generating a
random color. Of each example the first line is not using FlxRandom.color()
at all. The second line is using Steve's improved options method. The third
line is interpolating between colors given a colorModel (which defaults to
RGB).

// --- Example 1: Random color, no constraints (probably not often useful) ---
FlxColor.fromRGB(FlxRandom.int(0, 255), FlxRandom.int(0, 255), FlxRandom.int(0, 255));
FlxRandom.color();
FlxRandom.color();

// --- Example 2: RGB color with different properties for each ---
FlxColor.fromRGB(FlxRandom.int(100, 120), FlxRandom.int(110, 130), FlxRandom.int(115, 140));
FlxRandom.color({minRed: 100, maxRed: 120, minGreen: 110, maxGreen: 130, minBlue: 115, maxBlue: 140});
FlxRandom.color(FlxColor.fromRGB(100, 110, 115), FlxColor.fromRGB(120, 130, 140));

// --- Example 3: RGBA color with different properties ---
FlxColor.fromRGB(FlxRandom.int(100, 120), FlxRandom.int(110, 130), FlxRandom.int(115, 140), FlxRandom.int(100, 150);
FlxRandom.color({minRed: 100, maxRed: 120, minGreen: 110, maxGreen: 130, minBlue: 115, maxBlue: 140, alphaMin: 100, alphaMax: 150});
FlxRandom.color(FlxColor.fromRGB(100, 110, 115, 100), FlxColor.fromRGB(120, 130, 140, 150));

// --- Example 4: RGB color with constant range (not sure if this is actually very useful) ---
FlxColor.fromRBG(FlxRandom(100, 150), FlxRandom(100, 150), FlxRandom(100, 150));
FlxRandom.color({min: 100, max: 150});
FlxRandom.color(FlxColor.fromRGB(100, 100, 100), FlxColor.fromRGB(150, 150, 150));

// --- Example 5: Random red only ---
FlxColor.fromRGB(FlxRandom.int(110, 150), 100, 100);
FlxRandom.color({minRed: 110, maxRed: 150, min: 100, max: 100});
FlxRandom.color(FlxColor.fromRGB(110, 100, 100), FlxColor.fromRGB(150, 100, 100));

// --- Example 6: Ranged greyscale color ---
FlxColor.fromHSB(0, 0, FlxRandom.float(0.5, 1));
FlxRandom.color({greyscale: true, min: 128, max: 255});
FlxRandom.color(FlxColor.fromHSB(0, 0, 0.5), FlxColor.fromHSB(0, 0, 1), ColorModel.HSB);

// --- Example 7: Random hue and brightness, constant saturation ---
FlxColor.fromHSB(FlxRandom.float(100, 360), 1, FlxRandom.float(0, 1));
// Not possible with properties object method!!!
FlxRandom.color(FlxColor.fromHSB(100, 1, 0), FlxColor.fromHSB(360, 1, 1), ColorModel.HSB);

// --- Example 8: Random hue only ---
FlxColor.fromHSB(FlxRandom.float(100, 360), 1, 1);
// Not possible with properties object method!!!
FlxRandom.color(FlxColor.fromHSB(100, 1, 1), FlxColor.fromHSB(360, 1, 1), ColorModel.HSB);

In the above:-

In 4/8 cases, it is quickest not to use FlxRandom.color() at all

In 2/8 cases, the ColorModel method is only slightly better than no
FlxRandom.color()

In 2/8 cases, the existing method is quickest, but these arguably
give the least useful results.

in 2/8 cases, the existing method cannot be used at all!

Note also that the first and third methods tend to be shorter than the
existing FlxRandom.color() method, in spite of the presence of several
extra "Flx" prefixes.

In summary, I actually think FlxRandom.color() is a bit pointless, in any
implementation. I could only think of two cases where it provided any help,
but neither of these cases produce very useful results.

—
Reply to this email directly or view it on GitHub
#1158 (comment).

JoeCreates · 2014-06-07T17:04:29Z

@steverichey Actually, I think the best solution to most random color problems is to just use FlxColor and FlxRandom.int/float. As I touched on at the end of my previous post, the exception to this is when no constraints are required (for example, when testing). In the vast majority of cases, adding extra options to FlxRandom.color() actually makes for a more long-winded way of randomly generating a color than not using it at all (as illustrated in my examples).

It's worth noting that FlxRandom.color() has only recently become less useful, due to changes to FlxColor and the shorter names for FlxRandom.int() and FlxRandom.float(). Before these changes, it likely was the quickest way to randomly make colors with certain constraints, although limted to constraints that aren't so useful.

In all, my suggestion would be to keep the existing FlxRandom.color() purely for testing/prototyping, where the additional constraints aren't necessary. I suspect this constitutes the vast majority of existing uses. FlxRandom.colorExt() should probably just be deprecated/removed. All three implementations (current, yours, and mine) actually make for more work when you wish to use more constraints.

Removing colorExt would be a breaking change, though I kind of doubt is has ever been used that much. If you really wish to have that much control over the color you produce, then chances are you are not going to be wanting the lightness/brightness/saturation fluctuating all over the place. It's way more useful to use HSB/HSL, and now it is easier to use these color models than previously. If you really don't need much control over the color, then the existing color does the job pretty well.

In summary I think we should:-

Keep the old FlxRandom.color() for testing/prototyping when constraints aren't needed.
Remove, deprecate or ignore FlxRandom.colorExt(). It doesn't make things any easier.

steverichey · 2014-06-08T01:48:00Z

Having just used FlxColor.fromHSB(FlxRandom.int(0, 360), 1, 1); I could see the value in removing color() entirely from FlxRandom. I'd like to know what @Gama11, @gamedevsam, or any other interested parties think of that idea.

Gama11 · 2014-06-08T07:50:38Z

I like being able to do FlxRandom.color() to get any random color. That's a lot more verbose than it has to be with FlxRandom.int() + FlxColor.fromRGB(), as Joe's example shows.

Other than that, these are all good points. We should leverage the power of the new FlxColor instead of, in a way, dupliacting functianlity. I guess we could leave color() as is (with the old params?) and remove colorExt().

steverichey · 2014-06-10T13:28:35Z

@Gama11 I reverted color(). Ready to merge!

Gama11 · 2014-06-10T13:32:05Z

CHANGELOG.md

@@ -75,6 +75,7 @@
 * exposed currentSeed as an external representation of internalSeed
 * removed intRanged() and floatRanged(), int() and float() now provide optional ranges
 * removed weightedGetObject(), getObject() now has an optional weights parameter
+ * removed colorExt(), color() now accepts a ColorOptions object with a wide array of color settings


This is now outdated.

steverichey · 2014-06-10T13:45:53Z

Changelog always gets me! Fixed.

FlxRandom: Fixed getObject(), removed colorExt()

steverichey added 2 commits June 6, 2014 14:21

FlxRandom: Use typedef for color(), fix getObject error

30ce54a

fix merge conflict

5276b62

Gama11 reviewed Jun 6, 2014
View reviewed changes

fix per @Gama11

e919a4c

Gama11 reviewed Jun 6, 2014
View reviewed changes

remove unnecessary null check

1363cc6

Gama11 reviewed Jun 6, 2014
View reviewed changes

fix spacing

77e8ea8

Gama11 reviewed Jun 6, 2014
View reviewed changes

remove excessive nullable type specification from ColorOptions typedef

2317cf5

steverichey mentioned this pull request Jun 6, 2014

FlxRandom Demo Updates HaxeFlixel/flixel-demos#126

Merged

steverichey changed the title ~~FlxRandom: Fixed getObject(), made color() better.~~ FlxRandom: Fixed getObject(), made color() use a typedef for ease of use Jun 7, 2014

JoeCreates mentioned this pull request Jun 7, 2014

Use FlxColor for FlxEmitter/FlxParticle colors #1161

Closed

revert color()

53bc8a4

Gama11 reviewed Jun 10, 2014
View reviewed changes

update changelog

1e35228

Gama11 changed the title ~~FlxRandom: Fixed getObject(), made color() use a typedef for ease of use~~ FlxRandom: Fixed getObject(), removed colorExt() Jun 10, 2014

Gama11 added a commit that referenced this pull request Jun 10, 2014

Merge pull request #1158 from steverichey/getobjectfix

e49a024

FlxRandom: Fixed getObject(), removed colorExt()

Gama11 merged commit e49a024 into HaxeFlixel:dev Jun 10, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FlxRandom: Fixed getObject(), removed colorExt() #1158

FlxRandom: Fixed getObject(), removed colorExt() #1158

steverichey commented Jun 6, 2014

Gama11 Jun 6, 2014

steverichey Jun 6, 2014

Gama11 Jun 6, 2014

Gama11 Jun 6, 2014

steverichey Jun 6, 2014

Gama11 Jun 6, 2014

Gama11 Jun 6, 2014

steverichey Jun 6, 2014

JoeCreates commented Jun 7, 2014

steverichey commented Jun 7, 2014

JoeCreates commented Jun 7, 2014

steverichey commented Jun 8, 2014

Gama11 commented Jun 8, 2014

steverichey commented Jun 10, 2014

Gama11 Jun 10, 2014

steverichey commented Jun 10, 2014

FlxRandom: Fixed getObject(), removed colorExt() #1158

FlxRandom: Fixed getObject(), removed colorExt() #1158

Conversation

steverichey commented Jun 6, 2014

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

JoeCreates commented Jun 7, 2014

steverichey commented Jun 7, 2014

JoeCreates commented Jun 7, 2014

steverichey commented Jun 8, 2014

Gama11 commented Jun 8, 2014

steverichey commented Jun 10, 2014

Choose a reason for hiding this comment

steverichey commented Jun 10, 2014