Skip to content
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

quil.core.fill & stroke cast single integer arguments to float breaking tests #364

Open
nfletton opened this issue Jun 3, 2022 · 1 comment

Comments

@nfletton
Copy link

nfletton commented Jun 3, 2022

I'm running a local build of Quil based on master which uses Processing 4 beta 8. Commit 1f214e7, which is not yet in a released version of Quil, casts the argument of the single argument definition of fill and stroke to a float. Although this commit does make those functions conform to their documentation, it does break the common usage of those functions including in the Quil example snippets and hence tests fail.
As an example, the usage pattern below is now broken by the commit as the integer returned by (color) gets cast to a float before being passed to the underlying Processing method which interprets a single argument of type float as a gray scale.

(let [blue (q/color 156 255 70)]  
  (q/fill blue)  
  ...
  )

I suggest the single argument definition of fill and stroke should not cast to a float if the argument is of type integer.

@dgtized
Copy link
Contributor

dgtized commented Nov 19, 2023

I've started looking at this, but unfortunately it's a little more complicated than expected. Specifically because in Processing/clj q/color returns an unchecked int representing the current color, but in p5js/cljs q/color returns a color object. For p5js it also accepts a string, either as a hex or hsl/rgb css string (see fill, and color).

It's also not clear if q/color should always return a color mode translated RGB values, such that using an integer as an argument to fill, stroke, background should always assume RGB regardless of the color mode translation? If we don't translate it in fill, but only in color then it won't double translate it, if the argument is from q/color. However, that also means there is an escape hatch to force RGB color even if the mode is set to HSB/HSL or some other range of RGB.

Finally, if we call (q/background 128) or (q/stroke 255) in RGB, or HSL with integer range, should that be interpreted as grey scale or whatever int packed color that represents?

I'll try and make a table to summarize all the input modes to expected values, as I think that might help determine the best approach. I think your intuition on checking for int is correct, I just think we have a few more cases to cover for it to handle all the expected inputs in a sane way.

dgtized added a commit that referenced this issue Dec 8, 2023
Partial fix for #364. Forces any of the mulity-arity color functions to preserve
integers on gray or gray,alpha argument signatures.

This is sufficient to fix 5 tests so it's a good stop gap, but not happy with
performance and I still think a table of values and expected would help for this.
dgtized added a commit that referenced this issue Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants