Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#12827 closed enhancement (fixed)

Expand Animation class to accept more graphics types

Reported by: niles Owned by: jason, was
Priority: major Milestone: sage-6.2
Component: graphics Keywords: animate, graphics, 3D
Cc: gutow, nbruin, novoselt Merged in:
Authors: Niles Johnson Reviewers: John Palmieri
Report Upstream: N/A Work issues: docstrings, testing, think about img protocol
Branch: u/jhpalmieri/animate Commit: 599e40a73e4d355c3e31d18d61eb5d355964d39c
Dependencies: Stopgaps:

Description (last modified by niles)

Currently the Animation class only animates Graphics objects, and throws an error for Graphics3d, GraphicsArray, and Tachyon objects. The behavior of this class should be improved to accept any Sage object whose save method produces an image.

See this sage-devel thread for the beginning of this discussion, including some discussion of possible solutions.

Attachments (6)

trac_12827_animate3d.patch (17.5 KB) - added by niles 7 years ago.
uses more robust type-checking for inputs
trac_12827_animate3d_img_method.patch (19.1 KB) - added by niles 7 years ago.
add img method to the four major graphics classes
trac_12827_animate3d_save_image.patch (24.3 KB) - added by niles 7 years ago.
a rudimentary save_image method
accumulation.gif (303.6 KB) - added by niles 6 years ago.
animated graphics array
torus_knot.gif (436.8 KB) - added by niles 6 years ago.
animated parametric curve
twisted_cubic.gif (1.6 MB) - added by niles 6 years ago.
animated tachyon scene

Change History (37)

Changed 7 years ago by niles

uses more robust type-checking for inputs

comment:1 Changed 7 years ago by niles

  • Cc gutow nbruin added

A first patch is up now, which makes the type-checking of the Animate class more inclusive. However I am now leaning toward a different idea: adding an img() method to each of the major graphics classes defined as follows:

def img(self, *args, **kwds):
    return self.save(*args, **kwds)

Then for the Animation class, or other situations where an image representation of an object is required, one would call the img() method, and reject the object if it has no such method. To handle new object types, one would implement an img() method for those types. As I understand them, this is more closely aligned with the principles of object-oriented programming, and thus more appropriate for Sage library code.

I would appreciate hearing further thoughts on the relative merits of the current patch v.s. the img() method idea, as well as thoughts on naming this method (other suggestions are save_as_bitmap() and render_image()). Maybe the name should be prefixed or suffixed with an underscore too?

Changed 7 years ago by niles

add img method to the four major graphics classes

comment:2 Changed 7 years ago by niles

  • Authors set to Niles Johnson
  • Work issues set to docstrings, testing, think about img protocol

The second patch is a replacement for the first, using the idea of an img() method instead of type-checking. All tests pass, but I haven't finished writing docstrings.

Patchbot: apply trac_12827_animate3d_img_method.patch

comment:3 follow-up: Changed 7 years ago by nbruin

OK, I'm afraid the following if a bit of distraction of your core goal. Without reading any documentation, I would expect G.img() to return an "image object", rather than the G.save_as_bitmap("filename"). Of course save_as_bitmap still needs to be told in what format the bitmap should be saved.

I'm not sure we have an appropriate "image object". We do have PIL in sage. Try import Image. Would it make sense to interface with their types for an "img" return value?

Obviously, having a "convert to bitmap object" protocol implemented for all graphics-type objects, together with a save-to-file option on the bitmap object, would give you the required infrastructure to produce animations. It may not be the most efficient (or even sufficiently efficient) way of doing it, though ...

I can understand if you don't want to mess with bitmap objects (whatever they should be) and just settle for a way to dump a bitmap representation of objects into a file. However, you might not want to use "img" then, reserving that word for when someone does want to do that.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by niles

Replying to nbruin:

Without reading any documentation, I would expect G.img() to return an "image object"

Hmm, yes I see your point. Thanks for the tip about PIL -- that package does seem to provide a pretty good Image object, which is created by reading an image. So one might do something like the following:

def img(self, filename, *args, **kwds):
    self.save(filename=filename, *args, **kwds)
    from PIL import Image
    im = Image.open(filename)
    return im

Maybe this is the wrong way to go about it though, since the save() and show() methods of the resulting Image object would re-save the image to a file, wasting a lot of time. Image also has the capability to read an image from a stream or a buffer, but I don't know how to create such things from Sage's graphics objects.

On the other hand, the current show() method of Sage's graphics objects saves the image to a file before showing it, so this implementation would be following that precedent.

A more rudimentary compromise might be to have a method which saves the image and returns the system path to the image:

def img(self, filename, *args, **kwds):
    self.save(filename=filename, *args, **kwds)
    return filename

At this stage, I've given three different possible img methods, and I can see now how each one establishes a different protocol. Since this ticket is probably not the right place to try to establish a protocol, I think I'll try to use a different name. I'm really having a hard time warming up to the term "bitmap" though. Wikipedia explains to me that it is essentially correct, although the image types we would use by default, like JPG, PNG, TIFF, are compressed bitmap images. What do you think of save_image instead?

comment:5 in reply to: ↑ 4 Changed 7 years ago by nbruin

Replying to niles:

... Maybe this is the wrong way to go about it though, since the save() and show() methods of the resulting Image object would re-save the image to a file, wasting a lot of time. Image also has the capability to read an image from a stream or a buffer, but I don't know how to create such things from Sage's graphics objects.

Well, in some cases this would be the best thing we have available. In the case of 3D, for instance, the rendering is done with the external program tachyon, which get a scene description in a file and saves a bitmap into another file. So in that case, going through an Image object for animate is very wasteful.

Googling shows that matplotlib can be used to produce the bitmap in-memory and directly feed into Image. Incidentally, if those files are written in /tmp (or any tmpfs filesystem), the files likely never hit disk. They'll just live in memory-based buffers.

A more rudimentary compromise might be to have a method which saves the image and returns the system path to the image:

def img(self, filename, *args, **kwds):
    self.save(filename=filename, *args, **kwds)
    return filename

I personally like the rule that routines used for their side-effect do not return a value. Especially when you have to specify the filename anyway, it doesn't really make sense to return it. A "success" return value could work, but raising an exception on failure is more pythonic.

At this stage, I've given three different possible img methods, and I can see now how each one establishes a different protocol. Since this ticket is probably not the right place to try to establish a protocol, I think I'll try to use a different name. I'm really having a hard time warming up to the term "bitmap" though. Wikipedia explains to me that it is essentially correct, although the image types we would use by default, like JPG, PNG, TIFF, are compressed bitmap images. What do you think of save_image instead?

I think it's an improvement over "save", because to me "save" indicates a reversible operation (via "load"). In the case here, the bitmap is a different kind of object from which the original object cannot be reconstructed. I think save_as_image is even clearer, but it's also longer and has more underscores.

In general, people would differentiate "bitmap" and "vector graphics" formats inside "image" formats (i.e., jpg, png versus pdf, ps, svg, disregarding that the latter can also contain bitmap information). On the application side, GIMP versus Inkscape or Photoshop versus Acrobat/Illustrator?.

For 3D scenes we currently have no way of creating vector graphics representations, although those would be quite useful for producing print-quality schematics. matplotlib can create excellent vector graphics as well as bitmaps.

If save_as_image supports a format specifier (and/or derives a default from inspecting the filename for known extensions) that also supports "eps, svg, inkscape" then I think it's a very good name. Not all object have to support all formats of course. If you are going to categorically restrict to bitmap formats (compressed or not), I think sage_as_bitmap is better.

Look at how matplotlib solves this problem. Staying compatible with them

comment:6 Changed 7 years ago by gutow

My take on this is that G.image() should return an image object.  I haven't looked at PIL, but I would expect an image object to have a publicly accessible .type value that tells the image type and a .data or .imgdata that contains the binary representation of the image.  I would suggest that we want graphics objects to have two routines G.image(type =XXX, height=XXX, width=XXX, kwds) and G.save_image(type=XXX, filename=XXX, height=XXX, width=XXX, kwds).  I suppose the height and width could have default values.  G.save_image should return a success value, but raise an exception on failure.  

Changed 7 years ago by niles

a rudimentary save_image method

comment:7 Changed 7 years ago by niles

  • Description modified (diff)
  • Status changed from new to needs_review

Thanks for all the feedback. Having thought through the issues here, I'd like to provide a minimal solution to the animation problem now which doesn't conflict with potential future improvements to the graphics objects. I've opted for a save_image method which just passes its arguments to the save method. This can save in bitmap or vector formats, depending on the capabilities of the save method, and sizing of the output image will depend on the save method too.

All tests pass in the affected files, and all of the documentation builds without warning; Nils or Jonathan, are either of you interested in reviewing this?

Patchbot: apply trac_12827_animate3d_save_image.patch

comment:8 Changed 7 years ago by novoselt

  • Cc novoselt added

comment:9 Changed 7 years ago by kcrisman

Perhaps one should even just accept graphics files like pngs? Just a thought.

comment:10 Changed 7 years ago by kcrisman

  • Status changed from needs_review to needs_work

Sadly, this doesn't apply to Sage 5.0 - probably because of the refactoring of the graphics code to be largely in sage/plot/graphics.py. But I really like this potential! See for instance this ask.sagemath.org question.

comment:11 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:12 Changed 6 years ago by niles

  • Branch set to u/niles/ticket/12827
  • Created changed from 04/10/12 19:05:41 to 04/10/12 19:05:41
  • Modified changed from 08/13/13 15:35:53 to 08/13/13 15:35:53

comment:13 Changed 6 years ago by git

  • Commit set to 4bd63210b646fa15f908d41fd7500e7f881a6058

Branch pushed to git repo; I updated commit sha1. New commits:

4bd6321terminology update
9df0a83terminology update
34405b9remove commented code, move author info to top

comment:14 Changed 6 years ago by niles

  • Description modified (diff)
  • Status changed from needs_work to needs_review

The new branch is ready for review! It rebases the previous patch to sage 5.13.beta4. It includes other minor improvements to documentation and internal methods.

With this patch, the Animate class can create an animation from any iterable of objects which have a save_image method. Such a method is added to several of the base classes for different graphics types: sage.plot.graphics.Graphics, sage.plot.graphics.GraphicsArray, sage.plot.plot3d.base.Graphics3d, sage.plot.plot3d.tachyon.Tachyon.

comment:15 Changed 6 years ago by niles

bumping this ticket -- anyone available for review?

comment:16 Changed 6 years ago by git

  • Commit changed from 4bd63210b646fa15f908d41fd7500e7f881a6058 to 4b7ba9e0012eb9868057c07ed8b4e37c27a48c94

Branch pushed to git repo; I updated commit sha1. New commits:

411c4b2Merge branch 'master' into ticket/12827
aa34923first try to fix doctest continuation and trailing whitespace
008685fanother doctest continuation fix
4b7ba9efew more fixes to trailing whitespace and doctest continuation

comment:17 Changed 6 years ago by git

  • Commit changed from 4b7ba9e0012eb9868057c07ed8b4e37c27a48c94 to de5349040ebab566d5a0e35d99284e628ab21080

Branch pushed to git repo; I updated commit sha1. New commits:

de53490minor tweak to intro and one example

Changed 6 years ago by niles

animated graphics array

Changed 6 years ago by niles

animated parametric curve

Changed 6 years ago by niles

animated tachyon scene

comment:18 Changed 6 years ago by niles

Attached some animated gifs to show what the new patch can do. Code is below. It's also worth mentioning that this patch fixes some doctest errors in the Animation.ffmpeg() method. These are caused by a change in the way tmp_filename works, and the way file extensions are handled ('.gif' v.s. 'gif') elsewhere in sage. These bugs cause animate to break when convert is not available but ffmpeg is (e.g. on the sage cell servers).

Examples that this patch makes possible:

Animate graphics arrays show accumulation of area under a curve.

f(x) = 8*(x-4)^2 - 25
F(x) = f.integrate(x)
a,b = 0,7
M = max((_.find_local_maximum(a,b)[0] for _ in (f,F)))
m = min((_.find_local_minimum(a,b)[0] for _ in (f,F)))
bounding_pts = point([(a,0),(b,0),(0,m),(0,M)],color='black')

def fplot(t):
    return plot(f,a,a + t*(b-a),fill=True) + plot(f,a,b) + \
        line([(a + t*(b-a),0),(a + t*(b-a),f(a + t*(b-a)))],thickness=2) + bounding_pts

def Fplot(t):
    return plot(F,a,a + t*(b-a),color='green') + \
        point((a + t*(b-a),F(a + t*(b-a))),color='green',size=15) + bounding_pts

accumulation = animate((graphics_array([fplot(t),Fplot(t)]) for t in sxrange(.01,1,.05)))
accumulation.show()

Animate parametric surfaces and curves to draw a torus knot.

var('u,v');
a,b = 2,1 # outer and inner radii
x = (a + b*cos(u))*cos(v)
y = (a + b*cos(u))*sin(v)
z = b*sin(u)
T = parametric_plot3d([x,y,z], (u,0,2*pi),(v,0,2*pi), opacity=.6,aspect_ratio=1) 

var('t');
c = (2*t,3*t)
s = [_.subs(dict(zip((u,v),c))) for _ in (x,y,z)]
curve = lambda d: parametric_plot(s,(t,0,2*pi*d),color='red',thickness=2,plot_points=400)

K = animate((T+curve(d) for d in sxrange(.01,1.1,.05)))
K.show()

Animate Tachyon scenes to show the twisted cubic.

def tw_cubic(t):
    q = Tachyon(camera_center=(-2,3,1),xres=500,yres=500)
    q.light((1,1,11), 1,(1,0,1))
    q.light((1,5,1), .5,(0,1,1))
    q.texture('mirror', ambient=0.05, diffuse=0.05, specular=.9, opacity=0.9, color=(.8,.8,.8))
    q.texture('w',color=(1,1,1))
    q.plane((0,0,-2),(0,0,1),'w')
    q.plane((0,-2,0),(0,1,0),'w')
    for i in srange(-1,t,0.05):
        q.sphere((i,i^2-0.5,i^3), 0.1, 'mirror')
    return q

W = animate([tw_cubic(t) for t in srange(-1,1.5,.1)])
W.show()

comment:19 follow-ups: Changed 6 years ago by jhpalmieri

First, overall it looks pretty good to me.

Second, a typo: on the third line of animate.py, "iteratable" should be "iterable".

Third, I'm having problems with ffmpeg; this is on OS X 10.9. To try to fix them, I installed the latest version (from the git repo) of ffmpeg, but the problems persist.

  • the -loop_output flag leads to the error Unrecognized option 'loop_output'. The man page says
           -loop_output number_of_times
               Repeatedly loop output for formats that support looping such as
               animated GIF (0 will loop the output infinitely).  This option is
               deprecated, use -loop.
    
    Of course, the man page doesn't say anything at all about -loop so I don't know exactly how to use it, but just changing -loop_output to -loop produces a valid animated file. (I also don't know how long the -loop option has been available, so if we switch to that, will we just be breaking old versions of ffmpeg, or will it affect fairly recent ones?)
  • the -g 3 option leads to Codec AVOption g (set the group of picture (GOP) size) specified for input file #0 (/Users/palmieri/.sage/temp/Macintosh-001b639d44a1.local/67838/dir_lolwRD/%08d.png) is not a decoding option. Removing this option completely works. (I also can't find it in the man page, so I don't know what it's supposed to do. The man page is really not very good.)
  • the -pix_fmt rgb24 option leads to Incompatible pixel format 'rgb24' for codec 'gif', auto-selecting format 'pal8'. I just deleted this option and it works. Should we do that, or use -pix_fmt rgb8 (for example) instead? The available formats may depend on how ffmpeg was installed, so it might be better to omit it and allow users to specify it using the ffmpeg_options argument.
  • finally, the gif file produced by ffmpeg (for example using the sines animation) looks pretty bad: the axes and sine waves are multi-colored, and not in a good way. Using ffmpeg to produce a .mov file or .avi file works well.

comment:20 in reply to: ↑ 19 Changed 6 years ago by niles

Thanks for taking a look!

Replying to jhpalmieri:

Second, a typo: on the third line of animate.py, "iteratable" should be "iterable".

ouch. Will fix.

Third, I'm having problems with ffmpeg; this is on OS X 10.9. To try to fix them, I installed the latest version (from the git repo) of ffmpeg, but the problems persist.

hmmm, these are frustrating problems although it sounds like they exist independently of this patch. I'll have to look into them tomorrow.

Perhaps we could check the ffmpeg version and raise an error if it's too old.

comment:21 Changed 6 years ago by git

  • Commit changed from de5349040ebab566d5a0e35d99284e628ab21080 to 3561ef7ea858e344a1ae25b071e1077f6f41927a

Branch pushed to git repo; I updated commit sha1. New commits:

3561ef7fix typo, add pix_fmt option for ffmpeg, switch loop_output to loop

comment:22 in reply to: ↑ 19 ; follow-up: Changed 6 years ago by niles

I've tested this now on two (recent) versions of ffmpeg. Here is the output of ffmpeg -version:

(A)

ffmpeg version 1.2.2
built on Jul 29 2013 08:29:33 with Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)

(B)

ffmpeg version git-2013-06-29-53fd1ab
built on Jun 28 2013 23:41:23 with gcc 4.4.7 (GCC) 20120313 (Red Hat 4.4.7-3)

Replying to jhpalmieri:

Second, a typo: on the third line of animate.py, "iteratable" should be "iterable".

fixed now


  • the -loop_output flag

It seems that this change occurred in summer 2011, appearing in ffmpeg version 0.9 -- here's the commit on github. I'm not sure of the best course, but the ffmpeg developers didn't make the change backward-compatible so I'm inclined to do the same and blame them. Checking the ffmpeg version is annoying because it can be a number like 1.2.2 or a string like git-2013-06-29-53fd1ab. Who knows if there are yet other ways that ffmpeg reports its version. My current best idea is to just add a note about version 0.9 or higher in the documentation -- I'll do that in the next commit.


  • the -g 3 option

I get no error with or without this option on my ffmpeg version (A), but do get the error with this option on version (B). After some googling, it seems that this option controls the number of frames between keyframes, which is useful to adjust for streaming video. Seems like this is not important enough to include as a default setting in sage, so I've dropped it in the previous commit.


  • the -pix_fmt rgb24 option

Note that this option is applied by sage only for gifs. For my version (A), I have no warning message at all. For (B) I have a similar warning message to yours, but it says "auto-selecting format 'rgb8'". The ffmpeg documentation (for what it's worth) says that ffmpeg will automatically choose a new value if the given one is not available (implying that it will not raise an error, only a warning).

Importantly, my version (A), but not (B), raises an error if this value is not set. It seemed like the best course of action would add this as an argument to Animate.ffmpeg so that it can have a reasonable default value but be easily adjusted depending on the installation. I've done this in the previous commit.


  • the gif file produced by ffmpeg

The examples I produce with version (A) look pretty good to me, but the ones produced with (B) are bad as you describe (the rotating ellipses example is worse; parts of the ellipses change color and disappear as they rotate). I don't know what to do about this though. Increasing the thickness of the plotted curves helped substantially, but the output was still not great. It's possible there are some ffmpeg options which could improve things, but I haven't been able to find them.

comment:23 Changed 6 years ago by git

  • Commit changed from 3561ef7ea858e344a1ae25b071e1077f6f41927a to ce94d84d5557cfa43fd9c732f9d55a997bbc7569

Branch pushed to git repo; I updated commit sha1. New commits:

ce94d84allow disabling of -loop and -pix_fmt options in Animation.ffmpeg

comment:24 in reply to: ↑ 22 Changed 6 years ago by niles

Replying to niles:

Replying to jhpalmieri:

  • the -loop_output flag

My current best idea is to just add a note about version 0.9 or higher in the documentation -- I'll do that in the next commit.

I had a better idea while working on this: the user can set iterations=None and/or pix_fmt=None to stop sage from doing anything at all with these options. Then they can be set to whatever the user wants with ffmpeg_options. In this way, we support recent (as of summer 2011) versions of ffmpeg by default, but allow motivated people to work with an older version. If pix_fmt somehow causes trouble in a way I don't foresee, it can be disabled too.

The previous commit passes all (optional=sage,ffmpeg) tests in animate.py with both of my ffmpeg installations.

comment:25 Changed 6 years ago by jhpalmieri

  • Branch changed from u/niles/ticket/12827 to u/jhpalmieri/animate
  • Commit changed from ce94d84d5557cfa43fd9c732f9d55a997bbc7569 to 599e40a73e4d355c3e31d18d61eb5d355964d39c
  • Reviewers set to John Palmieri

Okay, I'm happy with your changes. Very nice. I've made a few very small modifications; if you're happy with them, we can switch this to "positive review".


New commits:

599e40aanimate.py: minor fixes

comment:26 Changed 6 years ago by niles

Your minor fixes look good to me -- I didn't know about the :trac: syntax. I'll let you flip the review switch.

comment:27 Changed 6 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

comment:28 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:29 Changed 6 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:30 Changed 5 years ago by kcrisman

Niles, is it possible you caused some problems here? Check out #16533 and friends - do animations still appear in the notebook for you?

comment:31 Changed 5 years ago by gagern

The issues described in #16570 are caused by tese modifications here as well. The problem here ist that one of the examples from the test suite looks ugly, due to xminand xmax no longer being passed to plot.plot. Nothing an automated test run would catch, but the manual review for this whole ticket here didn't work as well as it should have, imho.

Note: See TracTickets for help on using tickets.