`JImage` Implementation for J-Objects

[TheCedarPrince]

trafficstars

PR Checklist

If you are contributing to Javis.jl, please make sure you are able to check off each item on this list:

• [ ] Did I update CHANGELOG.md with whatever changes/features I added with this PR?
• [x] Did I make sure to only change the part of the file where I introduced a new change/feature?
• [ ] Did I cover all corner cases to be close to 100% test coverage (if applicable)?
• [x] Did I properly add Javis dependencies to the Project.toml + set an upper bound of the dependency (if applicable)?
• [ ] Did I properly add test dependencies to the test directory (if applicable)?
• [x] Did I check relevant tutorials that may be affected by changes in this PR?
• [x] Did I clearly articulate why this PR was made the way it was and how it was made?

Link to relevant issue(s) Closes #388

How did you address these issues with this PR? What methods did you use?

This is a proposed implementation idea for adding images to J-Objects easily. I was inspired by some of the conversation in #388 and wanted to create something easily like

function JImage(....)
     placeimage(....)
end

But could not get this syntax to work as when you have two separate objects - say JCircle which clips a circle shape and JImage - the clipping does occur but as JCircle does not "see" JImage, the expected output (i.e. the JImage is not clipped in a circle shape) does not occur. This PR introduces syntax such that you can write:

Object(
    JImage(
        pos = O,
        img = readpng("my_picture.png"),
        centering = true;
    ),
)

Which acts as a fully qualified Javis object (i.e. you can perform actions with it). Furthermore, clipping and outlining can be supported with the following kwarg syntax:

Object(
    JImage(
        pos = O,
        img = readpng("my_picture.png"),
        centering = true;
	shapeargs = (pt = O, r = 40, action = :clip),
	shape = circle,
    ),
)

So this picture will be cliipped by passing shapeargs and a Luxor shape function.

Picture of cat from Twitch Artist ChargedAsylum and owner AftonSteps

Disclaimer

I have not added tests or docs as of yet as I am waiting to see what the thought is with this possible syntax.

Pinging @Wikunia and @Sov-trotter for thoughts!

[codecov[bot]]

Codecov Report

Merging #399 (7ffc1ba) into master (23d7f89) will decrease coverage by 0.50%. The diff coverage is 0.00%.

:exclamation: Current head 7ffc1ba differs from pull request most recent head da0184c. Consider uploading reports for the commit da0184c to get more accurate results

@@            Coverage Diff             @@
##           master     #399      +/-   ##
==========================================
- Coverage   96.13%   95.63%   -0.51%     
==========================================
  Files          35       36       +1     
  Lines        1527     1535       +8     
==========================================
  Hits         1468     1468              
- Misses         59       67       +8     

Impacted Files	Coverage Δ
src/Javis.jl	96.36% <ø> (ø)
src/shorthands/JImage.jl	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 23d7f89...da0184c. Read the comment docs.

[TheCedarPrince]

@Wikunia and @Ved-Mahajan - I think the interface for @JImage is mostly stabilized to something that looks like this:

Object(
    JImage(
        pos = O,
        img = readpng("my_picture.png"),
        centering = true;
	shapeargs = (pt = O, r = 40, action = :clip),
	shape = circle,
        scaleargs = .5
    ),
)

What is left to do is add documentation and tests as of this moment. Anything else you guys can think of?

[Wikunia]

Sorry I kinda missed your example from the PR text. I think that looks great. I was wondering whether we could combine it with the other J objects such that one has a dispatch option to pass those as the shape. That might make it easier for the user and we would be able to call the appropriate :clip by default and determine the size by calling it with :path I assume as in those J Objects the action is a keyword argument. I would still keep the current form you have though to keep the flexibility. As a user I would find the other one easier to write though. Something like:

Object(
    JImage(
        pos = O,
        img = readpng("my_picture.png"),
        centering = true;
	shape = JCircle(0, 40),
    ),
)

[Wikunia]

You should be able to use Luxor 2.16 now and use action as a kwarg. Then scaling could be done automatically.

[TheCedarPrince]

This is nearly ready for merging, but somehow the CI is failing and I am having trouble running the test suite locally on my own machine. Otherwise, docs have been added, tests have been made, and the functionality is working. Should also explain what scaleargs can expect from scale.

Thoughts @Wikunia ?

[TheCedarPrince]

Hey @Wikunia , yes, that would help here as well as @cormullion pointing out a nice method that can help with automatic scaling! Planning to hopefully look at this a bit today!

[cormullion]

I think using bounding boxes might be an easy way to fit images into shapes. Here's some pseudo code:

# determine  BoundingBox:

bbox = BoundingBox(box(O, 100, 100)   # a simple rect

center = Point(60, -20)
radius = 120
bbox = BoundingBox(box(center, 2radius, 2radius)) # or a circle

bbox = BoundingBox(star(O, 100, 6, 0.5, vertices=true)) # or a poly

bbox = BoundingBox(first(pathtopoly())) # or the current path

# get image

img = readpng(...)

# calculate a scale factor, after determining whether you're fitting the 
# image (to see all of it) or filling (to occupy all the shape):

imagefit = true
if imagefit
    boxside = max(boxwidth(bbox), boxheight(bbox))
    imageside = max(img.width, img.height)
else
    boxside = min(boxwidth(bbox), boxheight(bbox))
    imageside = min(img.width, img.height)
end 
scalefactor = boxside/imageside

# draw clipping shape here

# finally place the image 
@layer begin
    translate(boxmiddlecenter(bbox))
    scale(scalefactor)
    placeimage(img, centered=true)
end

Not sure if this reliably works in all situations or in your Javis framework though... :)

[TheCedarPrince]

Hey @cormullion , Just responding more directly here to your thoughts. That pseudocode looks really great and actually very much like what I was hoping was available. I just wasn't sure how to get the bounding box of "shape" functions. As of right now, I need to double check but basically in my mind, any Luxor "shape" function (like, star, box, circle, ellipse, poly, etc.) should be able to support getting a bounding box. Then, a generalized scale factor could be calculated to either fill or fit the picture (or a user could provide their own scaling should they so choose).

[codecov-commenter]

Codecov Report

Merging #399 (5811f1a) into master (c99ad54) will decrease coverage by 69.33%. The diff coverage is 80.00%.

@@             Coverage Diff             @@
##           master     #399       +/-   ##
===========================================
- Coverage   96.26%   26.92%   -69.34%     
===========================================
  Files          36       35        -1     
  Lines        1634     1608       -26     
===========================================
- Hits         1573      433     -1140     
- Misses         61     1175     +1114     

Impacted Files	Coverage Δ
src/Javis.jl	53.74% <ø> (-42.31%)	:arrow_down:
src/shorthands/JImage.jl	80.00% <80.00%> (ø)
src/scales.jl	0.00% <0.00%> (-100.00%)	:arrow_down:
src/backgrounds.jl	0.00% <0.00%> (-100.00%)	:arrow_down:
src/structs/Layer.jl	0.00% <0.00%> (-100.00%)	:arrow_down:
src/structs/RFrames.jl	0.00% <0.00%> (-100.00%)	:arrow_down:
src/structs/LayerCache.jl	0.00% <0.00%> (-100.00%)	:arrow_down:
src/structs/PlutoViewer.jl	0.00% <0.00%> (-100.00%)	:arrow_down:
src/structs/LayerSetting.jl	0.00% <0.00%> (-100.00%)	:arrow_down:
src/Shape.jl	0.00% <0.00%> (-97.06%)	:arrow_down:
... and 19 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c99ad54...5811f1a. Read the comment docs.

[TheCedarPrince]

Hey @cormullion ,

So, I ran into the problem that I have been running into with different shape type functions. That is, for example, box, will always return a vector of Points that can be used for constructing a bounding box. However, a circle, will only return a Bool statement which cannot be used to build a BoundingBox to my knowledge. Is there any way to construct a BoundingBox for shapes such as these? Thanks!

P.S. Here are the shapes I have found that do not support building a BoundingBox.

• ellipse(centerpoint::Point, w, h; action)
• circle(centerpoint::Point, r; action)
• rect(xmin, ymin, w, h, action)

And instead return a Bool. Would you accept a PR that might fix this?

[cormullion]

"I don't want a Boolean!" 😃😃😃😃

Unfortunately circle() talks to Cairo which internally generates a set of four Bezier curves but returns nothing, so there aren't any coordinates to return...

So I've added a return statement to circle() that consists of a tuple of top left and bottom right corners of the bounding box of a circle:

julia> circle(O, 10, :path)
(Point(-10.0, -10.0), Point(10.0, 10.0))

So you can now go:

@draw begin
    pt1, pt2 = circle(O, 100, :stroke)
    box(BoundingBox(pt1, pt2), action=:stroke)
end 

or even

@draw begin
    circle(O, 100, :stroke)
    bbox = BoundingBox(circle(O, 100))
    box(bbox, :stroke)
end

and similarly for ellipse and rect:

@draw begin
    bbox = BoundingBox(ellipse(O, rand(1:200), rand(1:200), :stroke))
    box(bbox, :stroke)
end

I suppose these are breaking changes (if anyone is relying on circle returning a Boolean...!). Perhaps we'll go for Luxor 3.0!

Please check it out on the current Luxor!

[TheCedarPrince]

@Wikunia - I have a potentially radical new proposal based on some thoughts you had earlier for this PR:

What if instead of adding a JImage function, I instead create a dispatch around the existing class of J-Shapes (i.e. JBox, JCircle`, etc.) to enable automatic scaling of images and image placement? What do you think? I personally think this would make the syntax a lot less clunky and code a lot easier to read.

[TheCedarPrince]

• [X] JBox
• [x] JCircle
• [x] JEllipse
• [X] JPoly
• [x] JRect
• [X] JStar

Hey everyone! After much revision and review, I did decide to try overhauling the syntax I was proposing for JImage! Instead, I decided what would be best is to build on the syntax of the J-Objects. Therefore, we can get rather understandable syntax that looks something like this:

Object(
    31:45,
    JPoly(
        [Point(-100, 0), Point(0, -100), Point(100, 0), Point(80, 100), Point(-80, 100)],
        "white",
        1,
        :fill,
        true,
        false,
        "../test/refs/dispatch.png",
        :inset,
    ),
)

As I was working through this syntax, it made me wonder if @cormullion , would you be opposed to me making a Luxor PR to allow image placement inside of shapes such as Circle, Rect, and more? It's a bit odd as this syntax would be specific to only Javis so it would feel nice to merge this functionality upstream and we pull it in from Luxor. What do you think? Feel free to look at the file, "src/shorthands/JBox.jl` for the wrapping mechanism I use.

@Wikunia , what do you think of this syntax? It would also work for one to provide their own scaling factor if the options of :clip or :inset are not satisfactory.

Finally, here is code and a gif demonstrating some of the functionality:

Object(1:15, JBox(O, 200, 200, "white", :clip, false, "../test/refs/dispatch.png", :inset))

Object(
    16:30,
    JStar(O, 200, "white", 1, 5, 0.5, 0, :fill, false, "../test/refs/dispatch.png", :clip),
)

Object(
    31:45,
    JPoly(
        [Point(-100, 0), Point(0, -100), Point(100, 0), Point(80, 100), Point(-80, 100)],
        "white",
        1,
        :fill,
        true,
        false,
        "../test/refs/dispatch.png",
        :inset,
    ),
)

Also of note, this functionality will require the latest version of Luxor which I know will break morphing sadly..... :cry:

[Wikunia]

Why does the latest Luxor break morphing? The syntax looks good to me. One problem I see with the positional arguments is that I have no idea what the two boolean arguments are. Seems like you haven't pushed your changes yet so I'm not sure whether they are introduced in this PR or whether they already exist in the J functions in general. Maybe we can change the image filename argument in such a way though that it is a keyword argument.

[cormullion]

@cormullion , would you be opposed to me making a Luxor PR to allow image placement inside of shapes such as Circle, Rect, and more?

Haha, yes, probably (opposed), initially! 🤣🤣 Well, you'd have to persuade me - it would certainly be something that would need a fair bit of justification, to take account of the increased complexity, maintenance burden, introduction of (more) inconsistencies, testing, documentation, and so on., assuming there's no performance impact. But if you make a testable PR (with some benchmarks), we could discuss it...

[TheCedarPrince]

Hey @Wikunia !

This is ready for review! Let me know what you think of the syntax! Temporarily, I have just been relying on positional arguments, but we can change that in the future after your review to make the functionality much more readable.

Here is sample code to use to test out this feature's functionality:

using Javis

video = Video(400, 400)
origin(Point(200, 200))
function ground(args...)
    background("black")
    sethue("white")
end

Background(1:90, ground)

Object(1:15, JBox(O, 200, 200, "white", :clip, false, "../test/refs/dispatch.png", :inset))

Object(
    16:30,
    JStar(O, 200, "white", 1, 5, 0.5, 0, :fill, false, "../test/refs/dispatch.png", :clip),
)

Object(
    31:45,
    JPoly(
        [Point(-100, 0), Point(0, -100), Point(100, 0), Point(80, 100), Point(-80, 100)],
        "white",
        1,
        :fill,
        true,
        false,
        "../test/refs/dispatch.png",
        :inset,
    ),
)

Object(46:60, JEllipse(O, 200, 100, "white", 1, :fill, "../test/refs/dispatch.png", :inset))

Object(61:75, JCircle(O, 100, "white", 1, :fill, "../test/refs/dispatch.png", :inset))

Object(76:90, JRect(O, 200, 100, "white", 1, :fill, "../test/refs/dispatch.png", :inset))

render(video; framerate = 5, pathname = "test.gif")

Also, @cormullion - that makes complete sense. I will finish prototyping this functionality back here in Javis and then after we have something stable, I'll see about maybe making an example upstream PR to Luxor. Thanks!

P.S. @Sov-trotter - I am pinging you as well as this is somewhat altering the utilization of your J-Objects syntax. Wanted to know if that is ok with you! Thanks!

[Wikunia]

I currently can't run your test as I get the error which also seems to appear in the test cases:

MethodError: no method matching Luxor.BoundingBox(::Bool)

[Wikunia]

Make sure to update the Project.toml and specify that the master version of Luxor is needed or which version exactly.