first attempt at DelayedPosition #434

gpucce · 2021-10-20T14:53:29Z

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 #433

How did you address these issues with this PR? What methods did you use?
I added a new struct called DelayedPosition a new constant const STARTED_RENDERING = [false] and function get_delayed_position which is almost a DelayedPosition constructor.

Then I added a method to get_position(dp::DelayedPosition) that checks if the rendering has started, using the STARTED_RENDERING constant and then if it is called for the first time sets dp.position = get_position(dp.obj) and returns dp.position, otherwise just returns dp.position.

all together this should allow smth like this

testvideo = Video(200, 200)
Background(1:30, (args...) -> begin; background("white"); sethue("black") end)
ball = Object(JCircle(Point(-50, 0), 10, color="black", action=:fill))
act!(ball, Action(anim_translate(Point(-50, 0), Point(50, 0))))
act!(ball, Action(15:30, anim_translate(Javis.delayed_pos(ball), Point(0, -50))))
render(testvideo)

to return smth like this where the second upward shift starts from the circle position at the frame when the action with get_delayed_position starts:

Problems
I had to add DelayedPosition amone the field types of Translation and among the argument types of anim_translate and this might be needed else where I still don't know.

Wikunia

Looks good to me so far. Does that mean we don't need this Future approach? Or more or less: This is the future approach but called differently? I'm thinking we might want something like this also for rotation which is much less used though

Wikunia · 2021-10-20T14:57:58Z

src/Javis.jl

@@ -253,6 +259,8 @@ function render(
    postprocess_frames_flow = identity,
    postprocess_frame = default_postprocess,
 )
+
+    STARTED_RENDERING[1] = true


I think this needs to be set to false at the end again

I added it at the end, maybe it can be reset within the empy_CURRENT_constants() after naming it CURRENTLY_RENDERING.

once this works in general

changed STARTED_RENDERING to CURRENTLY_RENDERING

didn't move CURRENTLY_RENDERING[1] = false inside empty_CURRENT_variables because the latter is not called if pathname is empty, I don't know if there is a reason for that. if not I can move it inside it and make it a bit cleaner.

I think we should call it in all cases.

src/structs/Delayed.jl

src/structs/Transitions.jl

codecov · 2021-10-20T15:01:28Z

Codecov Report

Merging #434 (64c46ac) into master (86a3655) will decrease coverage by 0.47%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
- Coverage   96.19%   95.72%   -0.48%     
==========================================
  Files          36       36              
  Lines        1604     1613       +9     
==========================================
+ Hits         1543     1544       +1     
- Misses         61       69       +8

Impacted Files	Coverage Δ
src/object_values.jl	`64.51% <0.00%> (-22.45%)`	⬇️
src/Javis.jl	`96.06% <100.00%> (+0.03%)`	⬆️
src/structs/Transitions.jl	`94.11% <100.00%> (ø)`
src/shorthands/JLine.jl	`100.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 86a3655...64c46ac. Read the comment docs.

gpucce · 2021-10-20T15:04:06Z

Looks good to me so far. Does that mean we don't need this Future approach? Or more or less: This is the future approach but called differently? I'm thinking we might want something like this also for rotation which is much less used though

yeah this is the future approach while writing it I thought delayed was more understandable of course I switch it back if you think future is better

gpucce · 2021-10-20T15:25:38Z

I will look for a way to add it to Rotation as well

Wikunia · 2021-10-20T15:28:53Z

I think delayed makes perfect sense. I wonder whether we can abstract it away from the user though such that the user can still use pos for everything

gpucce · 2021-10-20T15:47:04Z

I think delayed makes perfect sense. I wonder whether we can abstract it away from the user though such that the user can still use pos for everything

Maybe one way could be differentiating based on when the structure is created. We add another struct called CurrentPosition on which pos acts like it does now on objects.

If pos(obj) is called at non render time it returns a DelayedPosition otherwise returns a CurrentPosition and then the following pos are called on these structures.

However, we would need to change the name of pos/get_position used by the user or rename the pos/get_postion inside util.jl to smth else.

codecov-commenter · 2021-10-26T10:50:59Z

Codecov Report

Merging #434 (207f03b) into main (2ed4d88) will decrease coverage by 0.03%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##             main     #434      +/-   ##
==========================================
- Coverage   96.47%   96.43%   -0.04%     
==========================================
  Files          35       36       +1     
  Lines        1617     1655      +38     
==========================================
+ Hits         1560     1596      +36     
- Misses         57       59       +2

Impacted Files	Coverage Δ
src/structs/Layer.jl	`100.00% <ø> (ø)`
src/structs/Delayed.jl	`83.33% <83.33%> (ø)`
src/Javis.jl	`96.92% <100.00%> (+0.05%)`	⬆️
src/object_values.jl	`90.62% <100.00%> (+3.12%)`	⬆️
src/shorthands/JBox.jl	`100.00% <100.00%> (ø)`
src/shorthands/JCircle.jl	`100.00% <100.00%> (ø)`
src/shorthands/JEllipse.jl	`100.00% <100.00%> (ø)`
src/shorthands/JLine.jl	`100.00% <100.00%> (ø)`
src/shorthands/JPoly.jl	`100.00% <100.00%> (ø)`
src/shorthands/JRect.jl	`100.00% <100.00%> (ø)`
... and 2 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 2ed4d88...207f03b. Read the comment docs.

gpucce · 2021-11-06T22:19:31Z

@Wikunia I don't know what happened with codecov but I think it is disallowed on juliaanimators or smth like that. I think I sent you and @TheCedarPrince a the authorization to enable it.

gpucce · 2021-11-06T22:23:00Z

Thanks!

gpucce · 2022-02-22T23:03:28Z

@Wikunia as I told you on zulip I went on and added support for all shorthands except for @jshape but there is a way to do it i will add it some where.

However now we can do this

vid = Video(500, 500)
Background(1:200, (args...)->background("black"))
o1 = Object(JCircle(O, 30, color="red", action=:fill))
act!(o1, Action(anim_translate(Point(200, 0))))
o2 = Object(50:200, JStar(Javis.delayed_pos(o1), 20, color="blue", action=:fill))
act!(o2, Action(anim_translate(Point(0, 100))))
o3 = Object(100:200, JBox(Javis.delayed_pos(o2), 10, 10, color="orange", action=:fill))
	
Object(100:150, JLine(Javis.delayed_pos(o2), Javis.delayed_pos(o1), 
	color="white"))
act!(o3, Action(1:50, anim_translate(Javis.delayed_pos(o2), Javis.delayed_pos(o1) + Point(10, 10)))) 
# Nore that you can add point to delayed_pos :) 
render(vid, pathname="test_vid.gif")

And the result is this

Wikunia · 2022-02-23T06:55:39Z

Awesome! Thanks a lot. One small question: what exactly does the last action mean? It's always hard to reason about an anim translate when the starting position is not the position of the object itself, right?

gpucce · 2022-02-23T06:58:14Z

In this case it is the same position maybe?

In general it will work as if you had replaced the two DelayedPosition with the points where the other objects are at the time when the Animation starts, hopefully

Wikunia · 2022-02-23T07:01:24Z

I was wondering as the end position should be at the right of the circle center. It just takes the difference of end and start point of the two given which makes it hard to reason about but I think we don't need to worry about that here. Would just suggest that you use something else for test cases and documentation where it is clearer what should happen 😊

gpucce · 2022-02-23T07:03:55Z

Sure I will, though also in this case looks like it behaves the way it should, if you wonder why it does not reach the end of the white line exactly is because I added + Point(10, 10) in the action just yo show that it is possible!!

Wikunia · 2022-02-23T07:05:25Z

But o1 is the circle isn't it ? 🤣

gpucce · 2022-02-23T07:06:30Z

Yeah! But it will aim for the position of the circle at moment the yellow brick starts moving, this si the behaviour in general

gpucce · 2022-02-26T10:49:29Z

Hi @Wikunia it would be nice to have your review!!

I'll add the test to raise the codecov but there should be nothing really important missing there for you to review the PR!

gpucce · 2022-02-26T11:18:44Z

Coverage seems to be doing good as well!

Wikunia · 2022-02-26T11:22:46Z

Currently on the move. Will check later today 🙂

Wikunia

I made some small comments.
One question regarding the DelayedPosition struct.
Would it make sense to have the ability to get the x and y component with the .x / .y syntax directly?

I think it that case we might be able to change all function arguments in Luxor to support AbstractPoint and we wouldn't need to call this get_position function for each AbstractPoint we send over to Luxor?

I mean for now it's okay as it is and then we don't need to create another Luxor release but it might be reasonable for the future. Let me know what you think.

docs/src/howto.md

src/Javis.jl

src/structs/Transitions.jl

test/delayed.jl

gpucce · 2022-02-26T19:02:18Z

I made some small comments. One question regarding the DelayedPosition struct. Would it make sense to have the ability to get the x and y component with the .x / .y syntax directly?

I think it that case we might be able to change all function arguments in Luxor to support AbstractPoint and we wouldn't need to call this get_position function for each AbstractPoint we send over to Luxor?

I mean for now it's okay as it is and then we don't need to create another Luxor release but it might be reasonable for the future. Let me know what you think.

That would be amazing

gpucce · 2022-03-02T09:13:10Z

@Wikunia I should have addressed all the comments!

Wikunia

LGTM you might wanna read through the documentation @TheCedarPrince

Also make sure to mention the functionality in the changelog @gpucce.

first attempt at DelayedPosition

01a4819

Wikunia reviewed Oct 20, 2021

View reviewed changes

reset STARTED_RENDERING at the end of render

64c46ac

Merge branch 'master' into add_delayedposition

908819b

gpucce added 17 commits October 26, 2021 15:22

add types to DelayedPosition and add DelayedPosition to Rotation

7d2ab4e

add first test for delayed

c53da96

format

5e5d08b

fix testset name

dc47929

add .keep back

44807f6

fix test in util not cleaning images folder

473add2

adjust testing so .keep does not get removed

073a575

format

0003112

Merge remote-tracking branch 'upstream/master' into add_delayedposition

a5550c0

add test for rotation and initial support for layers

64f6f29

add +,- for betwenn delayedposition and poin

f2ce80d

format tests

3944a90

minor operation change

8e1c204

add test for +

a9d3837

add test for delayedposition +/- point both left and right

dd3e312

change from STARTED_RENDERING to CURRENTLY_RENDERING and add test for it

c9ff494

adjust to master

53a8107

Wikunia marked this pull request as ready for review November 6, 2021 22:22

gpucce added 5 commits February 24, 2022 13:42

add delayed_pos for layers

16b65d0

fix JEllipse and add more tests

507fbae

Change from PointOrDelayed to Luxor.AbstractPoint

b7a205b

format

f1fb9e2

Minor changes

e0b6cb7

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

gpucce marked this pull request as ready for review February 26, 2022 10:48

gpucce requested a review from Wikunia February 26, 2022 10:49

gpucce added 2 commits February 26, 2022 11:59

reenable some unit tests

a309fb2

format

dfe7d49

Wikunia reviewed Feb 26, 2022

View reviewed changes

docs/src/howto.md Show resolved Hide resolved

docs/src/howto.md Outdated Show resolved Hide resolved

docs/src/howto.md Outdated Show resolved Hide resolved

src/Javis.jl Show resolved Hide resolved

src/structs/Transitions.jl Show resolved Hide resolved

test/delayed.jl Show resolved Hide resolved

gpucce added 4 commits March 2, 2022 10:05

export get_delayed_position

dce3bda

fix types

9a24a21

add docs to centered_point

a20bd27

fix example in howto.md

0b702a1

Merge remote-tracking branch 'upstream/main' into add_delayedposition

5be8a05

Wikunia approved these changes Mar 5, 2022

View reviewed changes

update changelog

207f03b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first attempt at DelayedPosition #434

first attempt at DelayedPosition #434

gpucce commented Oct 20, 2021

Wikunia left a comment

Wikunia Oct 20, 2021

gpucce Oct 20, 2021

gpucce Oct 20, 2021

gpucce Nov 4, 2021

Wikunia Dec 31, 2021

codecov bot commented Oct 20, 2021 •

edited

Loading

gpucce commented Oct 20, 2021

gpucce commented Oct 20, 2021

Wikunia commented Oct 20, 2021

gpucce commented Oct 20, 2021

codecov-commenter commented Oct 26, 2021 •

edited

Loading

gpucce commented Nov 6, 2021

gpucce commented Nov 6, 2021

gpucce commented Feb 22, 2022

Wikunia commented Feb 23, 2022

gpucce commented Feb 23, 2022 •

edited

Loading

Wikunia commented Feb 23, 2022

gpucce commented Feb 23, 2022

Wikunia commented Feb 23, 2022

gpucce commented Feb 23, 2022

gpucce commented Feb 26, 2022

gpucce commented Feb 26, 2022

Wikunia commented Feb 26, 2022

Wikunia left a comment

gpucce commented Feb 26, 2022

gpucce commented Mar 2, 2022

Wikunia left a comment

first attempt at DelayedPosition #434

Are you sure you want to change the base?

first attempt at DelayedPosition #434

Conversation

gpucce commented Oct 20, 2021

PR Checklist

Wikunia left a comment

Choose a reason for hiding this comment

Wikunia Oct 20, 2021

Choose a reason for hiding this comment

gpucce Oct 20, 2021

Choose a reason for hiding this comment

gpucce Oct 20, 2021

Choose a reason for hiding this comment

gpucce Nov 4, 2021

Choose a reason for hiding this comment

Wikunia Dec 31, 2021

Choose a reason for hiding this comment

codecov bot commented Oct 20, 2021 • edited Loading

Codecov Report

gpucce commented Oct 20, 2021

gpucce commented Oct 20, 2021

Wikunia commented Oct 20, 2021

gpucce commented Oct 20, 2021

codecov-commenter commented Oct 26, 2021 • edited Loading

Codecov Report

gpucce commented Nov 6, 2021

gpucce commented Nov 6, 2021

gpucce commented Feb 22, 2022

Wikunia commented Feb 23, 2022

gpucce commented Feb 23, 2022 • edited Loading

Wikunia commented Feb 23, 2022

gpucce commented Feb 23, 2022

Wikunia commented Feb 23, 2022

gpucce commented Feb 23, 2022

gpucce commented Feb 26, 2022

gpucce commented Feb 26, 2022

Wikunia commented Feb 26, 2022

Wikunia left a comment

Choose a reason for hiding this comment

gpucce commented Feb 26, 2022

gpucce commented Mar 2, 2022

Wikunia left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 20, 2021 •

edited

Loading

codecov-commenter commented Oct 26, 2021 •

edited

Loading

gpucce commented Feb 23, 2022 •

edited

Loading