-
Notifications
You must be signed in to change notification settings - Fork 435
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
FlxRandom: Fixed getObject(), removed colorExt() #1158
Conversation
@@ -217,10 +217,21 @@ class FlxRandom | |||
WeightsArray = [for (i in 0...Objects.length) 1]; | |||
} | |||
|
|||
if (StartIndex == null) |
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.
Wait.. why does this need to be nullable in the first place then? Why not just add a default value = 0
?
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 catch, fixed.
* @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. |
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.
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.
@@ -425,4 +448,19 @@ class FlxRandom | |||
return _recordingSeed; | |||
} | |||
#end | |||
} | |||
|
|||
typedef ColorOptions = { |
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.
Not sure if @gamedevsam is gonna like this, typedefs are fairly expensive on native. ;)
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.
Is there a better option? I just modeled this off of TweenOptions
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 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...
?min:Null<Int>, | ||
?max:Null<Int>, | ||
?alpha:Null<Int>, | ||
?greyscale:Bool, |
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.
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
.
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.
Removed! Thanks for pointing it out.
This is an improvement, though I have some further thoughts about 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, 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 Below are some comparisons of three different methods of generating a random color. Of each example the first line is not using
In the above:-
Note also that the first and third methods tend to be shorter than the existing In summary, I actually think 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 |
I agree that Personally, I use
|
@steverichey Actually, I think the best solution to most random color problems is to just use It's worth noting that In all, my suggestion would be to keep the existing Removing In summary I think we should:-
|
Having just used |
I like being able to do Other than that, these are all good points. We should leverage the power of the new |
@Gama11 I reverted |
@@ -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 |
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.
This is now outdated.
Changelog always gets me! Fixed. |
FlxRandom: Fixed getObject(), removed colorExt()
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. TheStartIndex
andEndIndex
parameters are now nullable integers, and if left unspecified, will be 0 andObject.length - 1
, respectively.Also, I had been frustrated with the complexity of
color()
andcolorExt()
, and finally implemented a single function (now justcolor()
) which accepts a singleColorOptions
object with all the settings previously individually set by each parameter.