Switching API to ints #83
Conversation
examples/circle_packing.ml
Outdated
| let resolution = (1200., 900.) | ||
| let tmap f (x, y) = (f x, f y) |
There was a problem hiding this comment.
Can we define resolution as a tuple of ints instead?
There was a problem hiding this comment.
If it were ints I would need to add 5-6 more calls to float_of_int, as opposed to the 1 call to tmap int_of_float resolution that's present.
There was a problem hiding this comment.
Where would that be required?
There was a problem hiding this comment.
Line 40 in rand_circle. I think this example needs to be rewritten from scratch though, it doesn't properly pack the circles since migrating to the Cairo backend and I think it could be made a bit more concise.
| ~c:(point (int_of_float x) (int_of_float y)) | ||
| (int_of_float radius))) |
There was a problem hiding this comment.
Can we instead change pack_circles and make_concentric to give us ints?
Also, instead of shrink, can you try using transform.scale to have the same effect?
There was a problem hiding this comment.
I can't use Transform.scale because I can't test the radii of the circles as they aren't exposed in the API. Also, that would require switching from the current representation of circles as (float * float) * float tuples.
I can make make_concentric and pack_circles take and return ints but that would require a lot of conversion back into floats because the math required to check if circles overlap is done with floats.
I think that, in general, this sketch is illustrative of what an end-user writing a more complex sketch will look like.
| let ctx = Cairo.create surface in | ||
| Cairo.scale ctx x y; | ||
| Cairo.scale ctx (float_of_int w) (float_of_int h); |
There was a problem hiding this comment.
What does this do?
If it scales un-evenly (w != h), would it convert our circles to ellipses?
There was a problem hiding this comment.
This just sets the initial scale to 1/1.
| @@ -1,6 +1,6 @@ | |||
| let context = Context.context | |||
|
|
|||
| type point = Shape.point | |||
| type 'a point = 'a Shape.point | |||
There was a problem hiding this comment.
What does this type change aim to do?
There was a problem hiding this comment.
Make it easier to convert between/handle the int points received from the end user
lib/render.ml
Outdated
| let radius = radius /. min (fst ctx.size) (snd ctx.size) in | ||
| let size = tmap float_of_int ctx.size in | ||
| let x, y = scale_point size c in | ||
| let radius = radius /. min (fst size) (snd size) in |
There was a problem hiding this comment.
Why do we change the radius like this?
I don't think this would give us what we want on a rectangular canvas.
There was a problem hiding this comment.
It has to be scaled down to a 0-1 range some way, I'm not sure what a better alternative is.
There was a problem hiding this comment.
I'm currently co-working with a friend who is a professional graphics programmer, she thinks it should be this radius /. (sqrt (Float.pow width 2. +. Float.pow height 2.) /. 2.)
There was a problem hiding this comment.
It depends on how we convert one unit of joy-land into one unit of cairo.
If you look at the new ticket I created - #86 - this is caused because we are converting a (float_in_range[-150..150], float_in_range[-250..250]) to (float_in_range[0..1], float_in_range[0..1]). It wouldn't be right to map a rectangle canvas to a square canvas. We should map to a rectangular canvas.
There was a problem hiding this comment.
I learnt that we don't need to map it to [0..1] either.
https://github.com/Chris00/ocaml-cairo/blob/1a347fb5d7cc887121bc335ecd3dfc68a79f0318/examples/stroke.ml#L6-L18
We can denormalize points to ([0..w], [0..h]) and that would be okay if we omit Cairo.scale w h later.
There was a problem hiding this comment.
I'm not sure I understand how that example shows that, the coordinates are all in [0, 1]
There was a problem hiding this comment.
The coordinates are scaled in [0..1] with respect to that axis. So scaling a Joy coordinate space of size [200, 300] to [0..1] on both axes does not scale it into a square aspect ratio. No resolution is 'lost', so to speak, as those [0..1] ranges mean different things for each axis.
This code also just gives us a static radius for any given circle, I feel confident that it isn't what's causing the distortions we're seeing in non-square aspect ratios.
|
Thanks! @FayCarsons This looks good to me. |
Switched API to ints, including colors and context initialization.