`JImage` Implementation for J-Objects #399

TheCedarPrince · 2021-08-14T18:43:30Z

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?
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)?
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)?
Did I check relevant tutorials that may be affected by changes in this PR?
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 · 2021-08-14T18:44:38Z

Codecov Report

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

❗ 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.

Sov-trotter · 2021-08-15T06:45:44Z

src/shorthands/JImage.jl

-        clipshape(clipargs...)
+function _JImage(pos, img, centering, shapeargs, shape)
+    if shape != false
+        shape(shapeargs...)


One minor thing:
I think we should mention(in the docstring) that shape should be a valid Luxor shape(a luxor function).
eg: rect, box, poly etc.

"valid Luxor shape" I'm not aware of this concept. :) Do you mean a built-in Luxor function - eg one that constructs paths, or one that generates points for polygons...? (Not trying to be awkward, just I get worried in case I'm not providing the tools you folks need for your cool constructions!)

Do you mean a built-in Luxor function - eg one that constructs paths

Yeah! We need shape to be a valid luxor function. Since a user may interpret it as entering a full shape name eg: rectangle instead of rect.

We mean any kind of function either their own or more likely a Luxor drawing function. Mostly those which provide the clipping action (so yeah a drawing function 😂)

OK, yes. Perhaps any function that has an action parameter/keyword...

Another thing I want to mention is that shape should by typed to Method.
eg: If there's a variable circle = Object(...), and user passes circle as an argument, it will throw the Objects of type Javis.Object are not callable error and maybe use a try catch block to give out a better error message?

Yea - error handling is something we need to reckon with @Sov-trotter . I agree with your thoughts and will see what I can do.

src/shorthands/JImage.jl

TheCedarPrince · 2021-09-28T02:27:35Z

@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

Some small comments for improvement and clarification/docstrings.
Can do another review after test cases are added.

src/shorthands/JImage.jl

Wikunia · 2021-09-28T14:53:36Z

src/shorthands/JImage.jl

+    if shape != false
+        shape(shapeargs...)
+    end
+    scale(scaleargs)


I would really like to have automatic scaling by checking the size of the shape and that of the image.
For the shape it would be awesome to be able to call it with :clip / :path automatically but it seems action isn't always a keyword argument which makes that a bit hard. Any idea on that @cormullion ?

yes, following on from my question (#399 (comment)) not all graphic construction functions are similar in having an action keyword. We'd have to compile a list and see which ones could be updated...

Are you interested in standardizing the functions though such that all have an option which provide using the action as a kwarg?

Perhaps we can make a list of possible functions as an issue at Luxor.jl. It might be easy or not, not too sure how many are affected...

JuliaGraphics/Luxor.jl#174

since this spawned off another issue, shall we resolve this comment @Wikunia and @cormullion ?

I decided to close this conversation as I do not think automatic scaling is feasible as all shapes can define a bounding box differently within Luxor.
To find that bounding box for a shape depends on the arguments provided by such a shape.

You can find a BoundingBox for a lot of "shapes" using eg pathtopoly():

@draw begin circle(O, 100, :stroke) circle(O, 100, :path) bbox = BoundingBox(pathtopoly()[1]) box(bbox, :stroke) end

If you can convert a shape to a polygon, then you can get a bounding box that way. Are there any other shapes in Luxor that you'd like BoundingBox to work on?

Wikunia · 2021-09-28T15:01:17Z

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),
    ),
)

src/shorthands/JImage.jl

Wikunia · 2021-10-09T09:53:58Z

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

TheCedarPrince · 2021-12-30T01:45:54Z

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 ?

Wikunia

Just some minor things. I assume #455 fixes your test cases

Wikunia · 2021-12-31T18:21:27Z

test/shorthands.jl

+            true;
+            shape = circle,
+            shapeargs = nothing,
+            scaleargs = 1,


Maybe add another test case with different shapeargs and different scale args.

Wikunia · 2021-12-31T18:22:28Z

src/shorthands/JImage.jl

+- `img::String`: Expects the path to an image
+- `centering::Bool`: Centers the object at `pos`
+- `shapeargs`: Arguments to be passed to a given shape type
+- `shape`: A Luxor shape function such as `circle`, `box`, etc.


would have shape before shapeargs and maybe add an example which shows what shapeargs and scaleargs do?

TheCedarPrince · 2021-12-31T19:01:06Z

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 · 2022-01-01T12:08:46Z

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 · 2022-01-03T04:16:00Z

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 · 2022-01-05T03:44:30Z

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%)`	⬇️
src/shorthands/JImage.jl	`80.00% <80.00%> (ø)`
src/scales.jl	`0.00% <0.00%> (-100.00%)`	⬇️
src/backgrounds.jl	`0.00% <0.00%> (-100.00%)`	⬇️
src/structs/Layer.jl	`0.00% <0.00%> (-100.00%)`	⬇️
src/structs/RFrames.jl	`0.00% <0.00%> (-100.00%)`	⬇️
src/structs/LayerCache.jl	`0.00% <0.00%> (-100.00%)`	⬇️
src/structs/PlutoViewer.jl	`0.00% <0.00%> (-100.00%)`	⬇️
src/structs/LayerSetting.jl	`0.00% <0.00%> (-100.00%)`	⬇️
src/Shape.jl	`0.00% <0.00%> (-97.06%)`	⬇️
... 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 · 2022-01-09T03:01:42Z

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 · 2022-01-09T11:17:47Z

"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 · 2022-01-10T02:04:22Z

@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 · 2022-01-13T00:25:16Z

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..... 😢

Wikunia · 2022-01-13T06:46:57Z

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 · 2022-01-13T11:31:26Z

@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 · 2022-01-17T01:01:27Z

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 · 2022-01-19T18:40:51Z

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 · 2022-01-19T19:31:36Z

src/shorthands/JCircle.jl

@@ -39,3 +39,58 @@ JCircle(p1::Point, p2::Point; kwargs...) =
    JCircle(midpoint(p1, p2), distance(p1, p2) / 2; kwargs...)

 JCircle(radius::Real; kwargs...) = JCircle(O, radius; kwargs...)
+
+"""
+TODO: Add Documentation!


currently I have no idea what :inset and :clip mean to be honest 😄
Seems like :clip positions the image somewhere else but I don't really have the option to move the image or have I?
Also for me a scale_factor is a number and not a symbol. Maybe scale_type or something but I would first need to know what they do to give my comment on that 😄

Wikunia · 2022-01-19T19:31:47Z

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

TheCedarPrince added 2 commits August 14, 2021 14:23

Exported the JImage function from the JImage shorthand

8f62037

Possible implementation of adding an image easily to JObjects

f1e0822

TheCedarPrince added the enhancement New feature or request label Aug 14, 2021

Made argument names less confusing

e1f5cd8

Sov-trotter reviewed Aug 15, 2021

View reviewed changes

Wikunia reviewed Aug 16, 2021

View reviewed changes

src/shorthands/JImage.jl Show resolved Hide resolved

TheCedarPrince added 2 commits September 27, 2021 20:43

Merge branch 'master' into tcp-image-jobjects

de8f54f

Added scaling capabilities

7ffc1ba

TheCedarPrince requested a review from Wikunia September 28, 2021 02:21

Wikunia requested changes Sep 28, 2021

View reviewed changes

Wikunia mentioned this pull request Sep 28, 2021

Action as keyword argument JuliaGraphics/Luxor.jl#174

Closed

9 tasks

TheCedarPrince added 2 commits October 4, 2021 23:35

Added docstring, changed default for shape

e908f6d

In process of adding tests for JImage

da0184c

Wikunia reviewed Oct 5, 2021

View reviewed changes

src/shorthands/JImage.jl Show resolved Hide resolved

TheCedarPrince added 5 commits December 29, 2021 18:34

Merge branch 'master' into tcp-image-jobjects

1ac4713

Added type signature from Cairo

c891118

Reference image for JImage

27620ba

Fixed docstrings

80957f1

Revised using dispatch image

6017adc

Wikunia reviewed Dec 31, 2021

View reviewed changes

TheCedarPrince added 2 commits January 4, 2022 20:58

Merge branch 'master' into tcp-image-jobjects

10c9686

Added TODO for scaling logic

68a452a

TheCedarPrince added 2 commits January 4, 2022 22:38

Temporary disabling of tests

36bbcbd

Added TODO for tests, fixed test cases, and fixed references

5811f1a

TheCedarPrince added 2 commits January 9, 2022 20:29

Added individual tests; working on scaling

2debc17

Begin adding autoscaling support for JImage

545268c

TheCedarPrince added 6 commits January 16, 2022 18:37

Added image support dispatch for JBox

80ca14e

Added image support dispatch for JPoly

92f36ca

Added image support dispatch for JStar

6e91da7

Added image support dispatch for JRect

2c18b27

Added image support dispatch for JCircle

d6331a2

Added image support dispatch for JEllipse

51dde2c

TheCedarPrince marked this pull request as ready for review January 17, 2022 01:01

TheCedarPrince requested review from Wikunia and Sov-trotter January 17, 2022 01:01

TheCedarPrince mentioned this pull request Jan 17, 2022

Prepped for tagging a v0.8.0 release of Javis #458

Draft

7 tasks

Wikunia reviewed Jan 19, 2022

View reviewed changes

Wikunia changed the base branch from master to main February 25, 2022 18:36

JImage Implementation for J-Objects #399

Are you sure you want to change the base?

JImage Implementation for J-Objects #399

Conversation

TheCedarPrince commented Aug 14, 2021

PR Checklist

Disclaimer

codecov bot commented Aug 14, 2021 • edited Loading

Codecov Report

Sov-trotter Aug 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sov-trotter Aug 15, 2021 • edited Loading

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

TheCedarPrince commented Sep 28, 2021

Wikunia left a 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

Wikunia commented Sep 28, 2021

Wikunia commented Oct 9, 2021

TheCedarPrince commented Dec 30, 2021

Wikunia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCedarPrince commented Dec 31, 2021

cormullion commented Jan 1, 2022

TheCedarPrince commented Jan 3, 2022

codecov-commenter commented Jan 5, 2022 • edited Loading

Codecov Report

TheCedarPrince commented Jan 9, 2022 • edited Loading

cormullion commented Jan 9, 2022

TheCedarPrince commented Jan 10, 2022

TheCedarPrince commented Jan 13, 2022 • edited Loading

Wikunia commented Jan 13, 2022

cormullion commented Jan 13, 2022

TheCedarPrince commented Jan 17, 2022 • edited Loading

Wikunia commented Jan 19, 2022

Choose a reason for hiding this comment

Wikunia commented Jan 19, 2022

`JImage` Implementation for J-Objects #399

`JImage` Implementation for J-Objects #399

codecov bot commented Aug 14, 2021 •

edited

Loading

Sov-trotter Aug 15, 2021 •

edited

Loading

Sov-trotter Aug 15, 2021 •

edited

Loading

codecov-commenter commented Jan 5, 2022 •

edited

Loading

TheCedarPrince commented Jan 9, 2022 •

edited

Loading

TheCedarPrince commented Jan 13, 2022 •

edited

Loading

TheCedarPrince commented Jan 17, 2022 •

edited

Loading