Animations #1164

georgefst · 2023-10-17T14:37:45Z

This is a proof-of-concept, but the implementation is solid enough to merge. Follow-up work is tracked in #1173.

(note to self - if any significant changes are made here due to reviews, #1173 will probably need to be modified accordingly)

dhess · 2023-10-17T14:46:33Z

Is the goal of this PR to merge it into main, or is it just a playground for some ideas?

georgefst · 2023-10-26T15:00:45Z

Is the goal of this PR to merge it into main, or is it just a playground for some ideas?

To merge. This code is now pretty solid, so I see no harm in keeping it around, even if some of it will be rewritten as we change our implementation or the student-facing API.

The one unsatisfying part is that we're extremely limited in what sort of lambdas we can pass to our animate primitive (I'll expand on this shortly). This is relaxed by #1169, but that is, unlike this PR now, very much an unmergeable hack.

brprice · 2023-11-14T16:23:34Z

The one unsatisfying part is that we're extremely limited in what sort of lambdas we can pass to our animate primitive (I'll expand on this shortly). This is relaxed by #1169, but that is, unlike this PR now, very much an unmergeable hack.

Could you point to where you expanded on this?

(ETA: the explanation will be expanded on in the linked #1169 itself)

georgefst · 2023-11-14T16:24:55Z

The one unsatisfying part is that we're extremely limited in what sort of lambdas we can pass to our animate primitive (I'll expand on this shortly). This is relaxed by #1169, but that is, unlike this PR now, very much an unmergeable hack.

Could you point to where you expanded on this?

Where it says "(full explanation to be filled in imminently)"! Sorry, I'll nudge you when that's done.

primer/src/Primer/Builtins/Picture.hs

brprice · 2023-11-14T16:32:20Z

primer/src/Primer/Primitives.hs

+  Animate ->
+    (
+      [ c tInt -- Loop time, in seconds.
+      , c tInt `f` c tPicture -- A function from frame number to output.
+      ]
+    , c tAnimation
+    )


To check my understanding, we have

(from the commit message) a fixed 10fps

an argument for loop length

thus Animate n p will denote a gif of 10*n frames of 0.1s duration each, for a total of n seconds, and will call p with arguments 0,1,...,10*n-1 or maybe 1,2,...,10*n to compute each frame?

Also, it is set to loop forever

Maybe elaborate the comments here?

dhess · 2023-11-14T16:34:40Z

I believe you said at our last meet-up that a Primer primitive for float (or similar) might be helpful in this API. Can you comment briefly here in the discussion thread on where/how?

brprice · 2023-11-14T16:36:01Z

primer/src/Primer/Primitives.hs

+  Animate -> case args of
+    -- Since we can only translate a `Picture` expression to an image once it is in normal form,
+    -- this guard will only pass when `picture` has no free variables other than `time`.
+    [PrimCon () (PrimInt duration), Lam () time picture]
+      | Just frames <- traverse diagramAtTime [0 .. duration * 100 `div` frameLength] ->


"can only": not true in single-step user-directed eval mode (though this is not exposed in the frontend currently)! I don't think this particularly matters, but maybe less confusing if you want to rephrase the comment to say something implying "we only support" rather than "we can only" (I initially read the comment as saying "it is only possible to ask to translate a picture when it is in normal form", rather than "you can ask, but we will just get stuck, if the argument is not in nf"). (This "only support" treatment is consistent with, say, primitive integers).

brprice · 2023-11-14T16:37:57Z

primer/src/Primer/Primitives.hs

+    -- Since we can only translate a `Picture` expression to an image once it is in normal form,
+    -- this guard will only pass when `picture` has no free variables other than `time`.
+    [PrimCon () (PrimInt duration), Lam () time picture]
+      | Just frames <- traverse diagramAtTime [0 .. duration * 100 `div` frameLength] ->


(Pedantic off-by-one): I think this length of frame indices is one too long (not that a human would be able to visually tell unless the construction function was wild)!

Good point. This made sense initially when I was thinking in terms of seconds rather than frame counts, and didn't anticipate looping. In which case we would dwell on the final frame. I changed my mind on those due to lack of floats and playback controls, respectively.

I've added a link back to this conversation from #1173, since we may well want to revert this if we do drop looping.

primer/src/Primer/Primitives.hs

brprice

(Untested) Code looks good to me, modulo some minor comments.

brprice · 2023-11-14T18:11:52Z

You say in a commit message

Using GIFs with tasty-golden for testing mostly works well, except that it has some issues displaying the ByteString in the terminal when the output differs from what is expected (outputs Exception: Cannot decode byte). But this is fine, since what we'd want to do in such a situation is to just pass --accept and compare the old and new GIF as rendered.

Two points here:

you don't seem to have actually added any tests
this seems like an odd problem, as tasty-golden treats files as binary. However, I think it is actually the upstream issue Non-textual output UnkindPartition/tasty-golden#51. If you agree, it is perhaps worth linking that

We have a new ADT, `Picture`, representing 2D images, and a primitive function `animate : Integer → (Integer → Picture) → Animation`, where `Animation` is a new primitive type, currently implemented as a base-64 encoding of a GIF for ease of use by clients. The arguments are a duration, in seconds, and a description of how to render each frame. This is inspired by the API of pure functional animation libraries such as Haskell's Gloss. We opt for keeping the API simple for now rather than providing full flexibility, so animations are always 10 frames/s on a low-resolution widescreen grid, looping forever (see `Values which are hardcoded`... comment). Both implementation and student-facing API are really just a proof-of-concept for exploring the use of Primer for effectful programming. Though much of what is added here (`Picture` ADT, new primitives etc.) is likely to remain useful for any future work in this direction. The one major current limitation is around the kinds of functions which we can pass as the second argument to `animate` without the evaluator getting stuck (`simple substitution hack`... comment). This is a high-priority task which will require some reworking of the evaluator, the details of which are as yet unclear. Using GIFs with `tasty-golden` for testing mostly works well, except that it has some issues displaying the `ByteString` in the terminal when the output differs from what is expected (outputs `Exception: Cannot decode byte`, which seems to be down to UnkindPartition/tasty-golden#51). But this is fine, since what we'd want to do in such a situation is to just pass `--accept` and compare the old and new GIF as rendered. Signed-off-by: George Thomas <[email protected]>

Note that we use `DisposalRestoreBackground` in encoding the GIF, whereas previously we indirectly used `diagrams-rasterific`'s default, `DisposalAny`. While that worked with the default black background, retaining it now would mean that we would see previous frames remaining in the background rather than disappearing. Signed-off-by: George Thomas <[email protected]>

brprice · 2023-11-20T13:40:25Z

primer/src/Primer/Typecheck.hs

-        unless (tc == tInt || tc == tChar) $ throwError' $ InternalError $ "Unknown primitive type: " <> show tc
+        unless (tc == tInt || tc == tChar || tc == tAnimation) $ throwError' $ InternalError $ "Unknown primitive type: " <> show tc
        let f b = case caseBranchName b of
              PatCon _ -> Nothing
              PatPrim pc -> case pc of


It might be nice here to link to a note giving an overview of the Animation type. In particular to explain why we do not allow pattern matching on it

I added this line yesterday to fix a test failure*, without initially looking at the context, which I was going to ask you about. But anyway, I've worked it out now, and realised the logic was actually wrong, and have now fixed this in 76d51cc.

I've also rewritten the whole passage in a form which I consider much easier to follow, and which would extend more easily to adding further primitives.

* This can be hit when cabal run primer-test -- -p "tcWholeProg idempotent" --hedgehog-replay "375/638:oAbA2d Seed 3320906864796586236 8917501983448077275"

Thanks for refactoring this, the new version is indeed much clearer

georgefst · 2023-11-21T13:46:24Z

I believe you said at our last meet-up that a Primer primitive for float (or similar) might be helpful in this API. Can you comment briefly here in the discussion thread on where/how?

This is mentioned several times in #1173. Essentially, all mentions of integer in this PR should be replaced with a real number representation, freeing students from thinking in terms of discrete time steps and drawing on fixed grid lines.

georgefst · 2023-11-21T13:47:02Z

Turns out I pushed the wrong branch after a rebase, which explains the missing tests, the empty module, and the negations on rectEnvelope being in the wrong commit. Sorry!

I've now applied all suggestions, and filled in #1169. I'm happy to merge this if there are no further objections.

Signed-off-by: George Thomas <[email protected]>

georgefst changed the title ~~WIP~~ Animations Oct 17, 2023

georgefst force-pushed the georgefst/animations branch from b18ffca to 1a943c7 Compare October 17, 2023 14:39

georgefst force-pushed the georgefst/animations branch 6 times, most recently from 078536a to 0b1e1c8 Compare October 24, 2023 13:51

georgefst force-pushed the georgefst/animations branch 2 times, most recently from fa26026 to ad9148d Compare October 24, 2023 15:47

georgefst marked this pull request as ready for review October 24, 2023 15:50

georgefst force-pushed the georgefst/animations branch 5 times, most recently from b2ec4fe to c7e473f Compare October 24, 2023 20:09

georgefst force-pushed the georgefst/animations branch 5 times, most recently from 0360f3f to 24d070b Compare October 30, 2023 19:35

georgefst mentioned this pull request Nov 14, 2023

Animations #1173

Open

georgefst requested a review from a team November 14, 2023 14:09

georgefst force-pushed the georgefst/animations branch from 24d070b to 5d52e9c Compare November 14, 2023 14:09

brprice reviewed Nov 14, 2023

View reviewed changes

primer/src/Primer/Builtins/Picture.hs Outdated Show resolved Hide resolved

brprice reviewed Nov 14, 2023

View reviewed changes

primer/src/Primer/Primitives.hs Show resolved Hide resolved

brprice approved these changes Nov 14, 2023

View reviewed changes

georgefst force-pushed the georgefst/animations branch 4 times, most recently from 85d1f94 to 9a258af Compare November 20, 2023 12:25

georgefst added 2 commits November 20, 2023 13:15

georgefst force-pushed the georgefst/animations branch from 9a258af to 66c14ec Compare November 20, 2023 13:16

brprice reviewed Nov 20, 2023

View reviewed changes

georgefst mentioned this pull request Nov 21, 2023

Evaluate arbitrary lambdas in animation primitive #1169

Draft

refactor: Clarify primitive pattern typechecking logic

3897c97

Signed-off-by: George Thomas <[email protected]>

georgefst force-pushed the georgefst/animations branch from ce07999 to 3897c97 Compare November 22, 2023 10:43

georgefst enabled auto-merge November 22, 2023 10:43

georgefst added this pull request to the merge queue Nov 22, 2023

Merged via the queue into main with commit e8e9f14 Nov 22, 2023
2 checks passed

georgefst deleted the georgefst/animations branch November 22, 2023 10:58

dhess mentioned this pull request Nov 22, 2023

wasm32-wasi support #1179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Animations #1164

Animations #1164

georgefst commented Oct 17, 2023 •

edited

Loading

dhess commented Oct 17, 2023 •

edited

Loading

georgefst commented Oct 26, 2023

brprice commented Nov 14, 2023 •

edited

Loading

georgefst commented Nov 14, 2023

brprice Nov 14, 2023 •

edited

Loading

dhess commented Nov 14, 2023

brprice Nov 14, 2023

brprice Nov 14, 2023

georgefst Nov 21, 2023

brprice left a comment

brprice commented Nov 14, 2023

brprice Nov 20, 2023

georgefst Nov 21, 2023

brprice Nov 22, 2023

georgefst commented Nov 21, 2023

georgefst commented Nov 21, 2023

Animations #1164

Animations #1164

Conversation

georgefst commented Oct 17, 2023 • edited Loading

dhess commented Oct 17, 2023 • edited Loading

georgefst commented Oct 26, 2023

brprice commented Nov 14, 2023 • edited Loading

georgefst commented Nov 14, 2023

brprice Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

dhess commented Nov 14, 2023

brprice Nov 14, 2023

Choose a reason for hiding this comment

brprice Nov 14, 2023

Choose a reason for hiding this comment

georgefst Nov 21, 2023

Choose a reason for hiding this comment

brprice left a comment

Choose a reason for hiding this comment

brprice commented Nov 14, 2023

brprice Nov 20, 2023

Choose a reason for hiding this comment

georgefst Nov 21, 2023

Choose a reason for hiding this comment

brprice Nov 22, 2023

Choose a reason for hiding this comment

georgefst commented Nov 21, 2023

georgefst commented Nov 21, 2023

georgefst commented Oct 17, 2023 •

edited

Loading

dhess commented Oct 17, 2023 •

edited

Loading

brprice commented Nov 14, 2023 •

edited

Loading

brprice Nov 14, 2023 •

edited

Loading