Opened 9 years ago
Closed 8 years ago
#11313 closed defect (fixed)
Animated GIF plots should repaint bgcolor after each frame
Reported by: | kini | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | graphics | Keywords: | plot, animate, imagemagick |
Cc: | Merged in: | sage-5.0.beta1 | |
Authors: | Keshav Kini | Reviewers: | John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
As title. ImageMagick should be called with -dispose Background
, because when you create an animation, Sage first creates a bunch of frames, then uses ImageMagick to make an animated GIF out of them. If the frames are not all the same size, then ImageMagick determines the minimum size for the final GIF in which it is possible to accommodate all the frames. The default behavior is to simply draw frame n
over frame n-1
without erasing frame n-1
. When frame n
's image is smaller than frame n-1
's image, however, you get garbage in frame n
outside the borders of the original image used to create frame n
.
Before:
After:
The above image was produced by the following code:
t = var('t') def frame(theta): r = 2 x = r*(t - sin(t)) y = r*(1 - cos(t)) plot = parametric_plot((x, y), (t,0,theta), rgbcolor=hue(0.6)) krug = circle((2*theta, r), r) tocka = point((r*(theta - sin(theta)), r*(1 - cos(theta))), pointsize=30, color='red') return plot+krug+tocka; animacija = animate([frame(x) for x in srange(0.1, 5*pi, 0.1)]) animacija.show(delay=10, iterations=0)
(Thanks to user v-v
on Freenode.)
Apply trac_11313-imagemagick-options.patch to $SAGE_ROOT/devel/sage
.
Attachments (1)
Change History (24)
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Description modified (diff)
comment:3 Changed 8 years ago by
Does this slow anything down? animate
is already on the slow side...
comment:4 Changed 8 years ago by
This is a defect, not an enhancement, ticket, so I don't consider potential slowdown to be a problem with the patch. That aside, ImageMagick? is written in C and is pretty fast. If animate
is slow it's probably because Sage needs to create every frame separately before using ImageMagick? to put them together. I doubt this patch introduces any slowdowns.
To elaborate on what makes this a defect: when you create an animation, Sage first creates a bunch of frames, then uses ImageMagick? to make an animated GIF out of them. If the frames are not all the same size, then ImageMagick? determines the minimum size for the final GIF in which it is possible to accommodate all the frames. The default behavior is to simply draw frame n
over frame n-1
without erasing frame n-1
. When frame n
's image is smaller than frame n-1
's image, however, you get garbage in frame 'n' outside the borders of the original image used to create frame 'n'.
comment:5 Changed 8 years ago by
- Description modified (diff)
comment:6 Changed 8 years ago by
- Description modified (diff)
comment:8 Changed 8 years ago by
Can you provide code which illustrates the problem (before the patch) and its solution (after)?
comment:9 follow-up: ↓ 15 Changed 8 years ago by
comment:10 Changed 8 years ago by
Here we are:
t = var('t') def frame(theta): r = 2 x = r*(t - sin(t)) y = r*(1 - cos(t)) plot = parametric_plot((x, y), (t,0,theta), rgbcolor=hue(0.6)) krug = circle((2*theta, r), r) tocka = point((r*(theta - sin(theta)), r*(1 - cos(theta))), pointsize=30, color='red') return plot+krug+tocka; animacija = animate([frame(x) for x in srange(0.1, 5*pi, 0.1)]) animacija.show(delay=10, iterations=0)
Credit to user v-v
on freenode. This produces the image shown above on Sage 4.7 (for example on sagenb.org). My local alpha2 (old, I know...) does something kind of alarming... I'll test with the latest dev version once I get it built.
comment:11 Changed 8 years ago by
- Status changed from needs_review to needs_work
Minor point: the patch doesn't apply cleanly to Sage 4.7.1.rc0, but that's easy to fix. More importantly, with Safari 5.1 on a Mac, the area which is no longer part of the picture displays as a black box. On Firefox or Chrome, it's white (or maybe the default background color?), but it looks bad on Safari. I don't know if this is a bug in how Safari displays animated gifs or if we can avoid it somehow.
comment:12 Changed 8 years ago by
The "something kind of alarming" appears to be caused by #11170 - the GIF blows up in filesize by a factor of 50 and the aspect ratio is destroyed. I'll cross-post to that ticket...
As for Safari, no idea. I'll look into it.
comment:13 Changed 8 years ago by
comment:14 Changed 8 years ago by
Now that #11650 is in, here's a rebase on sage 4.8.alpha6.
How the above GIF looks after the patch:
On the imgur page you can see how the GIF's transparency and non-white backgrounds interact. Compare with the old behavior.
comment:15 in reply to: ↑ 9 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Whoops, the "before" image is a bit different now in sage 4.8.alpha6 than it was when I posted 9. See the description for the updated GIF.
comment:16 follow-up: ↓ 17 Changed 8 years ago by
- Milestone changed from sage-4.8 to sage-5.0
- Reviewers set to John Palmieri
- Status changed from needs_review to positive_review
This looks good to me. By the way, where is this syntax for defining strings
s = ('abc' 'def')
documented?
comment:17 in reply to: ↑ 16 Changed 8 years ago by
Here is the relevant section of the Python docs. Actually,
s = 'abc' 'def'
works too, but the parentheses in the patch force a line continuation.
Thanks for the review!
comment:18 Changed 8 years ago by
Changed commit message.
comment:19 follow-up: ↓ 20 Changed 8 years ago by
Wait, what? So now we're not putting the trac ticket number in the commit message? Hmm, I didn't realize that. Thanks for the heads up.
comment:20 in reply to: ↑ 19 Changed 8 years ago by
Replying to kini:
Wait, what? So now we're not putting the trac ticket number in the commit message? Hmm, I didn't realize that. Thanks for the heads up.
Funny how in the beginning when I was release manager, I had to convince everybody to put ticket numbers in the commit message and quite a few didn't do it (or worse: put the wrong ticket number). Since sage-4.7, the ticket numbers are added automatically but it seems that now, more people than before are actually adding the ticket number.
comment:21 follow-up: ↓ 22 Changed 8 years ago by
Of course, the best way would be to have people make commits to the tree, and then close the ticket with a reference to the commit, rather than making the commit upon release, when the ticket has been closed already. But for that I guess we need to implement the github-based sage --push/pull workflow Robert Bradshaw suggested, or something similar.
By the way, why not just check whether the ticket number is already in the commit message and not add it again if so?
comment:22 in reply to: ↑ 21 Changed 8 years ago by
Replying to kini:
By the way, why not just check whether the ticket number is already in the commit message and not add it again if so?
When this was discussed on sage-devel (please don't ask me to find the thread again), somebody suggested that having a uniform format
Trac #54321: change foo to bar
could be useful for scripts. I would find a version of "hg blame" with ticket numbers quite interesting. If somebody would implement this, then the standard-format ticket numbers would certainly be useful.
comment:23 Changed 8 years ago by
- Merged in set to sage-5.0.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
Any ideas on how to doctest this? Doesn't really seem possible, on first glance anyway...