Adding color handling #90

FayCarsons · 2024-01-19T03:06:03Z

My first attempt at a system for coloring shapes and setting background in init. Shapes now have a color field that is an 8bit RGB three-tuple. Shape constructors are the same, inserting black as default, but shapes can be piped to a new function with_color that returns a new shape with the color field set to the arg.

So creating a gray circle can look like this:

let c = circle 50 |> with_color (128, 128, 128)

Sudha247

Thanks! This is useful refactoring. Some comments below.

examples/circle_packing.ml

lib/color.ml

Sudha247 · 2024-01-19T04:51:03Z

lib/shape.mli

+type circle = { c : point; radius : float; color : Color.color }
+type ellipse = { c : point; rx : float; ry : float; color : Color.color }
+type polygon = { vertices : point list; color : Color.color }
+type line = { a : point; b : point; color : Color.color }


Ideally we need not expose the internals of shape types in the mli file. But, I see that they're used in the Transform module, so we can't do away with it. Makes me wonder whether we should put them all in the Shape module.

Put all the transforms in shape.ml? I don't think that would bulk it up too much so I wouldn't have a problem with it, but I think I do have a small preference for breaking up projects like this.

nikochiko · 2024-01-27T05:21:49Z

Thanks! This looks great!

I have suggested some renaming and refactor changes. Please go through the review.

nikochiko · 2024-01-27T04:49:46Z

examples/circle_packing.ml

@@ -7,7 +8,7 @@ let max_radius = 150.
 let num_circles = 5_000
 let max_attempts = 100_000
 let shrink_factor = 0.85
-let _ = Stdlib.Random.self_init ()
+let _ = Rand.self_init ()


This renaming seems unnecessary. Because Random is part of Stdlib, you can e.g. do Random.self_init. I would recommend that so it is easier to read for someone who knows the module (no need to wonder if this is part of the stdlib or a module defined in this repo).

I'm planning on entirely rewriting this example as it broke when we migrated to the Cairo backend. Will keep comments regarding it in mind, though

nikochiko · 2024-01-27T04:52:08Z

examples/circle_packing.ml

@@ -29,20 +30,21 @@ let palette =

 (* utility Functions *)

+let rand_nth coll = List.length coll |> Rand.full_int |> List.nth coll


What is coll here?

Can you think of a name so it is obvious what it stands for?

I would also refactor this to make the random selection clearer. Something like this:

let n = Random.full_int (List.length coll) in List.nth n coll

I like this blog's explanation on why it's helpful (find for Explaining variables) https://henrikwarne.com/2024/01/10/tidy-first/

nikochiko · 2024-01-27T05:01:21Z

examples/circle_packing.ml


 (* creates a circle with a random center point and radius *)
 let rand_circle () =
  let point = rand_point () in
-  (point, min_radius +. Stdlib.Random.float (max_radius -. min_radius))
+  (point, min_radius +. Rand.float (max_radius -. min_radius))


Same here as above, please use Random directly

nikochiko · 2024-01-27T05:04:22Z

examples/circle_packing.ml

+    List.map
+      (fun ((x, y), radius) ->
+        circle
+          ~c:(point (int_of_float x) (int_of_float y))


Is it okay to lose the accuracy here when converting from float to int?

nikochiko · 2024-01-27T05:09:18Z

lib/context.ml

@@ -65,3 +52,15 @@ let save () =

 let restore () =
  match !context with Some ctx -> Cairo.restore ctx.ctx | None -> fail ()
+
+let init_context background_color line_width ((x, y) : int * int) axes =


We shouldn't only have a type signature for one parameter.
That should be in the .mli file.

Also, if these are the dimensions of the canvas, w and h would be more consistent names - x, y for point coordinates, and w, h for canvas size

Ah yeah this was left over from the type inference system insisting that these were floats, I should just be able to remove it.

Will rename x,y as well

nikochiko · 2024-01-27T05:12:18Z

lib/context.mli

@@ -10,7 +10,7 @@ val fail : unit -> unit

 exception Context of string

-val init_context : float -> int * int -> bool -> unit
+val init_context : int * int * int * int -> float -> int * int -> bool -> unit


We should have a named type for int * int * int * int, but that doesn't have to be part of this PR.

This is an RGBA color which is only used in setting the background color. Which is why I was hesitant to give it an alias. I think our current color type should probably keep its name, any thoughts what this should be named?

This conflict is why I thought it would be better to deal with it later. This is OK for now and shouldn't block the merge.

Q - can we have only one color type for both background/fill that has RGBA parts, or is that not supported by Cairo for strokes?

My thought was that since the focus of this library is simplicity, making users deal with an alpha value(which is almost always going to be at 255/1.0) every time they want to set the color of a shape would be a nuisance.

In my experience, creative coding libraries have a 'set_color' function that takes RGB and maybe another that takes RGBA or just alpha.

Cairo (and I believe HTML canvas) does support both

FayCarsons · 2024-01-31T19:16:46Z

Applied changes, caught a couple of small issues, and added color mapping functions: map_color which applies a fn to both stroke and fill, and map_stroke/map_fill which apply a fn to specific color fields in a shape.

Sudha247 · 2024-02-02T09:30:27Z

lib/color.ml

+
+(** Converts RGB color into opaque RGBA color. 
+    For use w/ `Context.background` *)
+let opaque (r, g, b) = (r, g, b, 255)


Can we use the same function to create custom colours as well?

I'm not sure I understand what you're asking, this function is for setting the background color in the init function. To make it easier to use RGB colors, that are maybe being used elsewhere, as an opaque background color. It wouldn't work for adding stroke or fill fields because those don't have an alpha field.

Do you think there should be a color constructor function? I didn't add one because I thought a three-tuple was easy enough to construct.

Sudha247

Left the minor comment above, otherwise LGTM! 👍

A circle in a rectangular canvas should not become an ellipse.

* Fix scaling factor: shouldn't be sqrt * Format with dune * Add donut-with-scaling example

…ap fns for applying fns to those fields

FayCarsons · 2024-02-16T18:49:46Z

Fixed merge conflicts!

Sudha247 reviewed Jan 19, 2024

View reviewed changes

FayCarsons force-pushed the color-system branch from 1690e13 to b1a2518 Compare January 26, 2024 19:17

nikochiko requested changes Jan 27, 2024

View reviewed changes

Sudha247 reviewed Feb 2, 2024

View reviewed changes

Sudha247 approved these changes Feb 2, 2024

View reviewed changes

FayCarsons and others added 13 commits February 16, 2024 12:44

first draft color system

8e47537

rewriting examples

b9f7cdb

dune fmt

45c9111

add proposal for color transforms

dfc98e2

Add example with rectangular canvas (for testing) (Sudha247#87)

324fa1e

A circle in a rectangular canvas should not become an ellipse.

Fix scaling factor in scale transform (Sudha247#88)

de0a19b

* Fix scaling factor: shouldn't be sqrt * Format with dune * Add donut-with-scaling example

more merging ??

581fdd0

merging w/ main branch

31978ee

added stroke and fill fields, 'with' fns to add them to shapes, and m…

0ee3039

…ap fns for applying fns to those fields

applying suggestions and adding map_color fns

0254b24

remove map_color

1ab198b

change docstring phrasing, fix .mli files

625fdab

fixing merge issues

14d1f2d

FayCarsons force-pushed the color-system branch from c94f844 to 14d1f2d Compare February 16, 2024 18:49

nikochiko merged commit 7a0f23d into Sudha247:main Feb 16, 2024
2 checks passed

FayCarsons deleted the color-system branch March 22, 2024 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding color handling #90

Adding color handling #90

FayCarsons commented Jan 19, 2024

Sudha247 left a comment

Sudha247 Jan 19, 2024

FayCarsons Jan 19, 2024

nikochiko commented Jan 27, 2024

nikochiko Jan 27, 2024

FayCarsons Jan 27, 2024

nikochiko Jan 27, 2024

nikochiko Jan 27, 2024

nikochiko Jan 27, 2024

nikochiko Jan 27, 2024

nikochiko Jan 27, 2024

FayCarsons Jan 27, 2024

nikochiko Jan 27, 2024

FayCarsons Jan 27, 2024

nikochiko Jan 27, 2024

FayCarsons Jan 27, 2024

FayCarsons commented Jan 31, 2024

Sudha247 Feb 2, 2024

FayCarsons Feb 5, 2024 •

edited

Loading

Sudha247 left a comment

FayCarsons commented Feb 16, 2024

		@@ -29,20 +30,21 @@ let palette =

		(* utility Functions *)

		let rand_nth coll = List.length coll \|> Rand.full_int \|> List.nth coll

Adding color handling #90

Adding color handling #90

Conversation

FayCarsons commented Jan 19, 2024

Sudha247 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikochiko commented Jan 27, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FayCarsons commented Jan 31, 2024

Choose a reason for hiding this comment

FayCarsons Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Sudha247 left a comment

Choose a reason for hiding this comment

FayCarsons commented Feb 16, 2024

FayCarsons Feb 5, 2024 •

edited

Loading