Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Grid Lines Animation #18

Merged
merged 35 commits into from
Aug 5, 2020
Merged

[WIP] Grid Lines Animation #18

merged 35 commits into from
Aug 5, 2020

Conversation

TheCedarPrince
Copy link
Member

Hey @Wikunia ,

Working on creating the gridlines animation. So far, I have the idea of what I want in my head being:

  1. A function that calculates how many horizontal and vertical grid lines to make based on canvas size
  2. Maybe making a mutable line object as opposed to the line object provided by Luxor.jl
  3. Creation of zero-lines function dependent on canvas size
  4. Creating a function that can "grow" the lines for the grid
  5. Hooking into FFMPEG

Right now, I generated a gif with at least the basic idea of drawing a line:

grid_line

Not very thrilling yet and it is not using javis just yet as I am unsure of how to intelligently use it. More to come.

@codecov
Copy link

codecov bot commented Jul 26, 2020

Codecov Report

Merging #18 into master will increase coverage by 3.81%.
The diff coverage is 94.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   87.06%   90.88%   +3.81%     
==========================================
  Files           2        4       +2     
  Lines         201      329     +128     
==========================================
+ Hits          175      299     +124     
- Misses         26       30       +4     
Impacted Files Coverage Δ
src/Javis.jl 81.19% <85.71%> (+1.58%) ⬆️
src/morphs.jl 94.02% <94.02%> (ø)
src/backgrounds.jl 100.00% <100.00%> (ø)
src/svg2luxor.jl 95.91% <0.00%> (+1.02%) ⬆️

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 e2357af...0981064. Read the comment docs.

@TheCedarPrince TheCedarPrince added documentation Improvements or additions to documentation enhancement New feature or request labels Jul 26, 2020
@Wikunia
Copy link
Member

Wikunia commented Jul 27, 2020

Can you clarify what you mean by your second point of creating a mutable Line object? We should always make sure that we have different names then Luxor to not have ambiguity issues.
You probably want to have a look at the javis or the animate function in Luxor for ffmpeg

@Wikunia
Copy link
Member

Wikunia commented Jul 28, 2020

BTW your javis try failed because you tried to define two functions. The javis function only accepts a single function and then a couple of transformation structs if needed. The single function can take in the arguments movie, action and frame such that you in general have all the information needed to do whatever you like to do in this animation action if that makes sense.

Out of the top of my head without testing I would say that this will work:

javis(video,
Action(1:100, :line, line_move)
)

function line_move(video, action, frame)
    t = (frame-first(action.frames))/(length(action.frames)-1)
    line(Point(-200,0), Point(-200,0)+t*(Point(200,0)-Point(-200,0)), :stroke)
end

or something like that. This is of course the user unfriendly version which needs to be abstracted into an easy to use version for the grid animation.

@TheCedarPrince
Copy link
Member Author

I am going to continue hacking on this but we are making progress! We have a proof of concept gif!

grid_test

@Wikunia , I tried your proposed fix about attempting to change this code snippet in Javis.jl:

    filecounter = 1
    for frame in frames
        Drawing(video.width, video.height, "$(tempdirectory)/$(lpad(filecounter, 10, "0")).png")
        origin()
        start_translation = Point(gettranslation()...)
        # this frame needs doing, see if each of the scenes defines it
        for action in actions
            if frame in action.frames
                @layer begin
                    compute_transformation!(action, video, frame)
                    perform_transformation(action)
                    res = action.func(video, action, frame)
                    if res isa Point
                        trans = cairotojuliamatrix(getmatrix())*res
			trans.p -= start_translation
                        video.defs[action.id] = trans
                    end
                end
            end
        end
        finish()
        filecounter += 1
    end

However, though I was able to create the line gif above, I could not get all the tests to pass. Here is the error message from the testing:

Test Summary: | Pass  Total
O(logn)       |    1      1
┌ Warning: test fails because PSNR -19.72929573059082 < 25
└ @ ReferenceTests ~/.julia/packages/ReferenceTests/JDrHL/src/equality_metrics.jl:45
Test for "dancing_circles_16.png" failed.
Dancing circles: Error During Test at /home/src/Projects/javis/test/animations.jl:23
  Got exception outside of a @test
  You need to run the tests interactively with 'include("test/runtests.jl")' to update reference images
  Stacktrace:
   [1] error(::String) at ./error.jl:33
   [2] test_reference(::FileIO.File{FileIO.DataFormat{:PNG}}, ::Array{RGB{Normed{UInt8,8}},2}, ::Nothing, ::Nothing; kw::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /home/cedarprince/.julia/packages/ReferenceTests/JDrHL/src/test_reference.jl:140
   [3] test_reference(::FileIO.File{FileIO.DataFormat{:PNG}}, ::Array{RGB{Normed{UInt8,8}},2}, ::Nothing, ::Nothing) at /home/cedarprince/.julia/packages/ReferenceTests/JDrHL/src/test_reference.jl:100
   [4] test_reference(::String, ::Array{RGB{Normed{UInt8,8}},2}; by::Nothing, render::Nothing, kw::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /home/cedarprince/.julia/packages/ReferenceTests/JDrHL/src/test_reference.jl:90
   [5] test_reference(::String, ::Array{RGB{Normed{UInt8,8}},2}) at /home/cedarprince/.julia/packages/ReferenceTests/JDrHL/src/test_reference.jl:90
   [6] top-level scope at /home/src/Projects/javis/test/animations.jl:41
   [7] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.4/Test/src/Test.jl:1113
   [8] top-level scope at /home/src/Projects/javis/test/animations.jl:24
   [9] include(::String) at ./client.jl:439
   [10] top-level scope at /home/src/Projects/javis/test/runtests.jl:8
   [11] include(::String) at ./client.jl:439
   [12] top-level scope at none:6
   [13] eval(::Module, ::Any) at ./boot.jl:331
   [14] exec_options(::Base.JLOptions) at ./client.jl:264
   [15] _start() at ./client.jl:484
  
Test Summary:   | Error  Total
Dancing circles |     1      1
ERROR: LoadError: LoadError: Some tests did not pass: 0 passed, 0 failed, 1 errored, 0 broken.
in expression starting at /home/src/Projects/javis/test/animations.jl:23
in expression starting at /home/src/Projects/javis/test/runtests.jl:8
ERROR: Package Javis errored during testing

Although it is obviously not great, I wrapped the line where the error occurs with a try-catch block. Now, all the tests work. I figure while I work on extending the functionality of this animation, this will do while you can see where the error is coming from @Wikunia :

    filecounter = 1
    for frame in frames
        Drawing(video.width, video.height, "$(tempdirectory)/$(lpad(filecounter, 10, "0")).png")
        origin()
        start_translation = Point(gettranslation()...)
        # this frame needs doing, see if each of the scenes defines it
        for action in actions
            if frame in action.frames
                @layer begin
                    compute_transformation!(action, video, frame)
                    perform_transformation(action)
                    res = action.func(video, action, frame)
                    if action.id !== nothing && res !== nothing
                        trans = cairotojuliamatrix(getmatrix())*res
			try
				trans.p -= start_translation
			catch 
				continue
			end
                        video.defs[action.id] = trans
                    end
                end
            end
        end
        finish()
        filecounter += 1
    end

@TheCedarPrince
Copy link
Member Author

We got it @Wikunia ! 🎉 🎉 🎉

grid_test

I think this is ready for review now. I'll let you see how to fix the problem in src/Javis.jl. Once you make your comments or anything you are thinking about wanting to add, please let me know and then I can start merging it as a core functionality.

@TheCedarPrince TheCedarPrince self-assigned this Jul 30, 2020
@TheCedarPrince TheCedarPrince linked an issue Jul 30, 2020 that may be closed by this pull request
@Wikunia
Copy link
Member

Wikunia commented Jul 31, 2020

Great! Thanks for pinging me
Some general comments first while I'm on vacation 😊
My suggestion was to use if action.id !== nothing && res isa Transformation
The first part checks whether the action has a name and the second whether the action returned a transformation. Sorry I said Point before if that's the case then save the point to the name.
Additionally currently you haven't integrated it into Javis yet, right? You basically built it using Javis which is an important first step. Next up should be to make it simpler for the user to achieve the same thing and have flexibility as how many lines if not default, speed of animation, maybe direction of animation, etc.

@TheCedarPrince
Copy link
Member Author

Hey @Wikunia ,

That fix you proposed worked. All tests are now passing without any hacks going on! 🎉

As for adding additional functionality, I added the ability to draw zero lines. Furthermore, I have now added the ability to choose directions where grid lines and zero lines finish:

Bottom Right:

grid_test

Bottom Left:

grid_test

Top Right:

grid_test

Top Left:

grid_test

Current Problem

This is somewhat embarrassing but I cannot seem to add kwargs to the function definition of my draw_grid function.

using Javis
using Luxor

function grid_animation()

    video = Video(500, 500)
    javis(
        video,
        [
            Action(1:100, ground),
            # Action(1:100, :line, draw_grid(truth_value=true)),
            Action(1:100, :line, draw_grid),
            Action(1:100, :line, zero_lines),
        ],
        tempdirectory = "/home/src/Projects/javis/test/grid_test",
        creategif = true,
        pathname = "/home/src/Projects/javis/test/grid_test.gif",
    )

end

# function draw_grid(video::Video=nothing, action::Action=nothing, frame::Int=1, truth_value::Bool=false)
function draw_grid(video::Video, action::Action, frame::Int)

I want to have the two commented lines work by passing in a boolean from the javis function, but I cannot get it working. What am I missing? Thanks @Wikunia -- after I get this figured out, I will keep experimenting with this and add it into the src/Javis.jl file and will begin developing some tests. 😄

@TheCedarPrince
Copy link
Member Author

Hey @Wikunia , I believe this is fully ready to merge. I addressed all your prior points, added in some tests and reference images, made sure all the checks passed, improved the documentation and syntax, and applied the optimizations you suggested.

Think it is good to go? When you give me the OK, I'll merge it! 😄

src/backgrounds.jl Outdated Show resolved Hide resolved
src/backgrounds.jl Outdated Show resolved Hide resolved
src/backgrounds.jl Outdated Show resolved Hide resolved
src/backgrounds.jl Outdated Show resolved Hide resolved
src/backgrounds.jl Outdated Show resolved Hide resolved
src/backgrounds.jl Outdated Show resolved Hide resolved
@Wikunia
Copy link
Member

Wikunia commented Aug 2, 2020

I've added some more comments. For test cases you might want to test both functions in the same movie.
Additionally I aim for good coverage so the other 3 options should be tested as well if possible.

@Wikunia
Copy link
Member

Wikunia commented Aug 2, 2020

You have a lot of code reuse such that it might be helpful to think of horizontal lines and of vertical lines separately.
So instead of 4 "global" if you are better of with 2 ifs for the downward/upward motion and 2 if for left/right motion.

@TheCedarPrince
Copy link
Member Author

Almost done. Just finishing up with the closures!

src/backgrounds.jl Outdated Show resolved Hide resolved
src/backgrounds.jl Outdated Show resolved Hide resolved
src/backgrounds.jl Show resolved Hide resolved
@Wikunia
Copy link
Member

Wikunia commented Aug 5, 2020

Additionally make sure that you resolve your merge conflicts.

src/backgrounds.jl Outdated Show resolved Hide resolved
src/backgrounds.jl Show resolved Hide resolved
src/backgrounds.jl Show resolved Hide resolved
@Wikunia
Copy link
Member

Wikunia commented Aug 5, 2020

Thanks for the PR. Sorry for the long process but I'm sure it's getting faster the next times. Please feel free to merge this yourself.
I normally like to merge my own PRs when a reviewer accepts them. Same is true when you review my PRs. Sometimes I find myself having a new idea which I want to try before it gets merged 😉 Just some comments from my project management perspective

@TheCedarPrince TheCedarPrince merged commit 0191ddb into master Aug 5, 2020
@TheCedarPrince TheCedarPrince deleted the tcp-feature-2 branch August 5, 2020 20:19
@Wikunia Wikunia mentioned this pull request Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Gridlines Animation
2 participants