Closed ryanartecona closed 7 years ago
Hey @ryanartecona, thanks for the changes :) I actually just moved, so my apt doesn't have internet yet, I looked over your changes a bit on my phone but I'll look again more closely later. I kind of feel like it would be better to be consistent with the color values. Whether that means all float or all 0-255 I'm not entirely sure. Generally what I do in these design cases is check how Processing handles it. I'm also wondering if we might as well make the alpha mandatory in this case. It's not harder to type a 1. than a () and might be more clear to beginners to reason (which is one of the goals of this library). Another option could be to have another function like Utils.colorAlpha or something.
Re double painting.. yeah alpha support will probably make a couple of things double paint which hopefully isn't too terrible to fix.
Can you explain the dpi bug/fix? Couldn't see it in all the refmt. There was a bit of movement in reasongl lately that might have caused this. There were some broken things for a while but should ideally all be fixed now.
Is there a new version of refmt yet that handles comparison/equality operators better? The
if (a
== b) ...
is kinda awful lolol.
I've fixed up my changes in place to use a refmt width of 100 instead of the 80 used previously. It looks like it made some things a little better, when it could put stuff on a single long line, but there are still some...quirks. LMK if we should stick with 100 or I can downgrade reason-cli to 1.13.6 or something in the meantime.
The hidpi fix is in 83e3838.
Re the color API: I was unsure here too. Processing supports all these:
color(gray)
color(gray, alpha)
color(value1, value2, value3)
color(value1, value2, value3, alpha)
color(hex)
color(hex, alpha)
And all valueN
/gray
/alpha
components can either be an int or float. I deferred to CSS rgba(255,255,255,1.0)
-style syntax, but I have no real affinity.
More loose thoughts:
colorT
record have floats for all components instead of ints? It looks like GL eventually wants floats anyway, and it seems like using floats all the way through would allow for a larger color space than (implicitly-8bit) ints? I'm not a graphics programmer, so unsure if there's more to it than this.color
signatures should Reprocessing support? Just 1 color: r::float => g::float => b::float => a::float => colorT
(or using all ints, idc)? Helpers for gray, rgb (without alpha), or hexadecimal int colors? We've only had the 1 color
so far, but tbh it wasn't long as a Reprocessing user until I started writing my own color helpers.Also, no rush on my part for this! I've pulled in Reprocessing as a submodule on my project so I can dev on it easier and don't incur any pain waiting on PRs to merge :) Hope you're settling into your new place smoothly @Schmavery !
This looks great. I think having float API to be a number between 0 and 1 is good, but I think the purpose of this library is to be simple to grasp so I think calling the float color colorf
and having another one that takes 0-255 ints would be dope.
Apart from this I think we can merge! Awesome work :)
I don't mind the color/colorf idea, this matches what we've done elsewhere. I'll try to remember to pull tonight and try this out :)
@ryanartecona I tried out the hidpi fix and was able to confirm it worked on my end too. If we can switch to the color/colorf format, I'd love to merge this! We can ignore the refmt stuff for now, it'll get better with time.
Also, if you don't have time to do this, lmk and I'll merge and change it myself, np.
Great! I should have a chance to switch to a color/colorf interface this evening. I'll also go back to setting my refmt width to 80 and rebase those changes back in.
A couple last questions. If we're going with color/colorf, should the internal colorT use floats for all components instead of ints? I think that would slightly simplify some of the internal color component handling I noticed while in here.
I think there would be no problem with converting internal colorT to use floats but we can always do that as a separate change as it would probably require tweaking of a decent number of internal functions.
Ok, this should be good to go!
I went ahead and changed colorT to all float components since I had to update all call sites & examples anyway with the move to requiring an alpha arg. I did a spot check before & after with my project that uses Reprocessing, and nothing changed!
Thanks!
This adds an alpha channel to the color type and to Utils.color.
Caveats:
1.0
inUtils.color
. A side effect of this isUtils.color
now requires a()
to terminate the named args.Also, I fixed a hidpi bug (I think) that was making rendering weird on my new retina laptop (all rendering to my reprocessing env was constrained within the bottom left quadrant of the otherwise black canvas).