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

add semi-portable looping using fsnotify #213

Merged
merged 4 commits into from
Sep 10, 2014
Merged

Conversation

bergey
Copy link
Member

@bergey bergey commented Sep 7, 2014

generic defaultLoopRender for use in Backends

See matching branches of -cairo and -rasterific.

This seems to work well on Linux. It works some of the time on Windows; other times fsnotify seems to not register a change, or the program compiles and runs an older copy of the source file. Please don't merge until I have spent more time debugging that.

I'd appreciate if someone could test this on a Mac. The -rasterific branch should be fine. My test program is:

import Diagrams.Prelude
import Diagrams.Backend.Rasterific.CmdLine

dia :: Animation B R2
dia = c <$> ui where
  c t = circle 1 # fc (blend t white green) # pad 1.01

main :: IO ()
main = mainWith dia
  • compile test program
  • run as ./Animation -w 400 --fpu 5 -o animTest.png -l
  • edit source file at least twice to trigger recompilation
    (there were some earlier bugs where it would recompile after the first edit, but not again)

generic defaultLoopRender for use in Backends
@jeffreyrosenbluth
Copy link
Member

This looks really nice. I should have some time to test this on a Mac tomorrow.
Do you know if it works with hsenv?

@jeffreyrosenbluth
Copy link
Member

When i run your test program on my mac it generates the 5 pngs but then i get the message:

Animation: select: Invalid argument

and the program terminates. I'm running in a cabal sandbox, using cabal exec zsh.

@bergey
Copy link
Member Author

bergey commented Sep 8, 2014

I added some special support for sandboxes, but I expect it just works with hsenv. I'll verify that on Linux.

I don't know what that error is; I'll see if I can reproduce it, or at least figure out where it might come from, this evening. Thanks for testing!

@jeffreyrosenbluth
Copy link
Member

If i run it with -threaded i get:

Animation: c_poll: invalid argument (Invalid argument)
Animation: <file descriptor: 13>: hGetBuf: invalid argument (Bad file descriptor)
Animation: ioManagerWakeup: write: Bad file descriptor
Animation: ioManagerWakeup: write: Bad file descriptor

let frames = simulate (toRational $ animOpts^.fpu) anim
nDigits = length . show . length $ frames
forM_ (zip [1..] frames) $ \(i,d) -> mainRender (indexize out nDigits i opts) d
defaultAnimMainRender ::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a formatting change? (that's fine, I just want to make sure I'm not missing anything)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two let lines are just reformatted, but there's a significant change before and after. Sorry about that.

Instead of getting mainRender implicitly from the Mainable instance, the first argument is a rendering function. This lets me write a defaultAnimMainRender that doesn't know anything about looping, and a defaultLoopRender that doesn't know anything about animations, at the expense of longer Mainable instances. (See line 269 of diagrams/diagrams-cairo@ec32e9a) I like the modularity, but I could be convinced to use the approach in #195 instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. This is all a bit tricky. I think this approach works well, but I think we can do better (probably separate issue) with error messages. As it is right now if I give a file name that it can't use to pick the renderer I get an error message for each frame and it continues on to the looping. I think #195 has the same issue. My fault for not thinking through making errors modular as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I agree we can do better. Making a new issue for it sounds good.

@bergey
Copy link
Member Author

bergey commented Sep 9, 2014

@jeffreyrosenbluth That looks like https://ghc.haskell.org/trac/ghc/ticket/7325 . Is the right thing to pick a smaller delay value on MacOS? I have no confidence that the magic values for one machine will work on all machines, though.

Can you edit lib/src/Diagrams/Backend/CmdLine line 587 and replace maxBound with something substantially less than maxBound? The ticket linked has a test program for picking suitable values.

Does the old-style looping work? That calls threadDelay with 1e6 by default, but we shouldn't have to go that low. (maxBound is ~9e18 on my 64-bit machine.)

@jeffreyrosenbluth
Copy link
Member

@bergey works for <=1e16 and fails for >=1e17. I didn't bother trying to pinpoint a more exact value since as you said, I don't realy have much confidence in magic values. I think I would go with something like 1e12.

to work around https://ghc.haskell.org/trac/ghc/ticket/7325

fix lower bound on fsnotify—required for polling interval
@bergey
Copy link
Member Author

bergey commented Sep 10, 2014

Thanks! That's great.

@bergey
Copy link
Member Author

bergey commented Sep 10, 2014

I think this is ready to merge.

jeffreyrosenbluth added a commit that referenced this pull request Sep 10, 2014
add semi-portable looping using fsnotify
@jeffreyrosenbluth jeffreyrosenbluth merged commit 67ddcc1 into master Sep 10, 2014
@jeffreyrosenbluth jeffreyrosenbluth deleted the fsnotify branch September 10, 2014 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants