hi Charbel
I realize now this is quite a big patch; I did a more detailed review
(below) and have some comments about the design. Also, the recent
refactoring in master conflicts with your patch so it will have to be
rebased.
On Sun, Mar 28, 2010 at 08:02:46PM +0200, Charbel Jacquin wrote:
> Thanks you for your review. I've finally found the time to update my animation
> patch. [..]
>
> Apologies for the added json dependency, I'm working with python 2.6 where the
> module is builtin.
Good news then. We could also raise the required python version to 2.6, later.
> I have updated my patch to accomodate various flavors of
> json libraries so packager's hate should be moderate ;-)
That should be okay; working fine for me now.
> Same for pencil document interchange. Too much an hack for now (and in the
> long term, I think it would be much more useful to patch pencil to use ora
> files too).
We would have to discuss this on the create mailing list; there were already
some thoughts about using ora for animation, but quite differently I think.
The question is whether this should remain a mypaint-specific addition or
part of the ora standard to share with other applications. If your plan is
to share it, you should really bring the topic (and extended file format
proposal) to the create mailing list.
> All in all, we are left with a patch implementing IMHO a pretty consistent
> behaviour, both in animation mode and in standard painting mode:
>
> 29a8855e Flipbook animation mode for mypaint
>
> We should be pretty close now ...
Yes. Functionality is okay with me, I think the complexity will be managable
to maintain this feature. Your patch seems to be working quite robust.
Two comments about the GUI:
- I think we should not have the word "Animation" visible during normal
painting usage. It is okay for the development branch now, and to get
feedback; when more options are added to the layer dialog we can hide it
behind something more neutral like "Advanced" or "Options" that doesn't
scream "play around with animation now!" into the painter's face.
- After enabling animation mode, it is not obvious how to use it. I suggest
to turn the current layer into an animation layer, to encourage clicking
on the visibility column, or add a new empty animated layer or similar.
Some comments on the commit / code design:
On Sun, Mar 28, 2010 at 05:48:35PM +0200, Charbel Jacquin wrote:
> When in animation mode, empty layers are meaningful
Really? Why? I don't get that, even after reading the patch... and I see
some possibly confusing stuff with layers that can be None...
> and so are kept on save/load (the standard behaviour is to discard them).
There probably is no good reason for doing that anyway, except that it is
not clear what their bounding box is.
> diff --git a/gui/drawwindow.py b/gui/drawwindow.py
> index ce8ff0b..85d9c30 100644
> --- a/gui/drawwindow.py
> +++ b/gui/drawwindow.py
> @@ -426,18 +426,10 @@ class Window(windowing.MainWindow):
> self.app.filehandler.filename = None
>
> def layer_bg_cb(self, action):
> - idx = self.doc.layer_idx - 1
> - if idx < 0:
> - return
> - self.doc.select_layer(idx)
> - self.layerblink_state.activate(action)
> + self.app.layersWindow.layers_list.layer_bg_cb(action)
>
> [...] (same pattern repeats)
Hm... anything speaking against registering
app.layersWindow.layers_list.layer_bg_cb as callback directly?
> @drawwindow.with_wait_cursor
> def open_file(self, filename):
> try:
> - self.doc.load(filename)
> + saved_state = self.doc.load(filename)
I think this is where I disagree about the design choice. Everything that is
saved into the file shoud be part of the model (in the lib/ directory). The
save_state seems to be bypassing that concept.
> - self.doc.save(filename, **options)
> +
> + # CHECKME, 'options' usage
> + if not keywords:
> + options = {}
> + keywords = {'options': options}
> + else:
> + options = keywords.setdefault('options', {})
I think the else branch also does what you want if keywords is empty, no
need for that if.
> + try:
> + self.doc.save(filename, **keywords)
> + finally:
> + if layers.animation_mode:
> + layers.update_animation()
Do we really have to update the GUI after saving? Saving should never modify
the document...
> diff --git a/gui/layerswindow.py b/gui/layerswindow.py
> index 44aba70..0fcd3ef 100644
> --- a/gui/layerswindow.py
> +++ b/gui/layerswindow.py
> @@ -23,6 +23,19 @@ def stock_button(stock_id):
> return b
>
>
> +STICKY_VISIBLE = 0
> +STICKY_INVISIBLE = 1
> +ANIMATED =2
Those states are now part of our model and thus should not be defined in the
GUI code, but in lib/ somewhere.
> def draw(self,widget, event):
> @@ -69,7 +93,7 @@ class EyeOnly(gtk.DrawingArea):
> x0 = w/2.0
> y0 = h/2.0
> cr.set_line_width(1.4)
> - if self.active:
> + if self.visibility == STICKY_VISIBLE:
The terms "sticky_visible" and "sticky_invisible" are confusing. When I read
those, I don't expect them to stand for the normal case. They make sense
when talking about animation mode, but the code here mainly deals with the
non-animated case. Maybe just use visible/invisible/animated instead?
> class Eye(gtk.ToggleButton):
> - def __init__(self):
> + def __init__(self, layer=None):
> gtk.ToggleButton.__init__(self)
> self.set_active(True)
> self.eye = EyeOnly()
> - self.eye.set_active(True)
> + if layer:
> + state = get_layer_visibility(layer)
> + else:
> + state = STICKY_VISIBLE
> + self.eye.set_visibility(state)
> self.add(self.eye)
> self.connect('toggled', self.toggled)
Huh? Does not make sense to me to draw an eye widget without a layer
associated to it, what is that for?
> + def get_visibility(self):
> + return self.eye.visibility
> +
> + def set_visibility(self, state):
> + self.eye.set_visibility(state)
> +
> + def set_animation_mode(self, on):
> + self.eye.set_animation_mode(on)
Yuk, do-nothing methods... can we get rid of them maybe?
> class LayerWidget(gtk.EventBox):
> - def __init__(self, parent, layer=None):
> + def __init__(self, parent, layer=None, animation_mode=False):
After quickly skimming over the code, is layer=None really possible?
Maybe it is a relict? Seems to make things needlessly complicated.
> @@ -332,6 +408,197 @@ class LayersList(gtk.VBox):
> + def new_layer_cb(self, action):
> +
> + dw = self.app.drawWindow
> + doc = dw.tdw.doc
> + insert_idx = doc.layer_idx
> + if doc.layers:
> + current = doc.layers[insert_idx]
> + sticky, visible = current.sticky, True
> + else:
> + sticky, visible = True, True
> + if action.get_name() == 'NewLayerFG':
> + insert_idx += 1
> + doc.add_layer(insert_idx, sticky=sticky, visible=visible)
Hm... much code... I think I agree about moving this from drawwindow into
the layerslist. But it looks a bit complicated...
> + def layer_fg_cb(self, action):
> +
> + dw = self.app.drawWindow
> + doc = dw.tdw.doc
> +
> + layer_idx = doc.layer_idx
> + current_layer = doc.layers[layer_idx]
> + if self.animation_mode:
> + active_layers = [layer for layer in doc.layers if layer.sticky
> == current_layer.sticky]
> + layer_idx = active_layers.index(current_layer)
> + else:
> + active_layers = doc.layers[:]
> +
> + idx = layer_idx + 1
> + if idx >= len(active_layers):
> + if not self.loop_mode:
> + return
> + else:
> + idx = 0
> + next_layer = active_layers[idx]
> + doc_idx = doc.layers.index(next_layer)
> + doc.select_layer(doc_idx)
> + if not self.animation_mode:
> + dw.layerblink_state.activate(action)
> + else:
> + dw.tdw.queue_draw()
Also looks a bit complicated, but I see quite a few neat features that are
probably worth it.
> + def layer_bg_cb(self, action):
> [...]
Since the above is almost the same, and not trivial any more, maybe those
methods could be combined into one.
> + def update_animation(self):
> + current_layer = self._selected.layer
> + if current_layer.sticky:
> + return
> +
> + dw = self.app.drawWindow
> + doc = dw.tdw.doc
> + active_layers = [layer for layer in doc.layers if not layer.sticky]
> + current_idx = active_layers.index(current_layer)
> + before = current_idx - 1
> + after = current_idx + 1
> + count = len(active_layers)
> +
> + for layer in active_layers:
> + layer.onion_skin = 0.0
> +
> + active_layers[current_idx].onion_skin = 1.0
> +
> + if self.ghost_before:
> + if before >= 0:
> + active_layers[before].onion_skin = 0.5
> + elif self.loop_mode:
> + if count != 1:
> + active_layers[-1].onion_skin = 0.5
> +
> + if self.ghost_after:
> + if after < count:
> + active_layers[after].onion_skin = 0.5
> + elif self.loop_mode:
> + if current_idx != 0:
> + active_layers[0].onion_skin = 0.5
> +
> + dw.tdw.queue_draw()
To me this looks the wrong way around. Instead of setting an onion_skin
property for every layer, and updating it all the time, would it not be more
ellegant to decide whether a layer is onion_skin at display time?
Note that the layer can also be switched during an undo command, etc.
Calling update_animation everywhere seems a bit like a hack to me.
> + def save_state(self, options):
> + dw = self.app.drawWindow
> + doc = dw.tdw.doc
> + keep_empty_layers = options.get('keep_empty_layers', False)
> + if keep_empty_layers:
> + print "save state: keep_empty_layers"
> + saved_layers = doc.layers
> + else:
> + print "save state: discard_empty_layers"
> + saved_layers = [layer for layer in doc.layers if not
> layer.surface.is_empty()]
> + layer_states = [ (layer.sticky, layer.visible) for layer in
> saved_layers ]
> + if self.selected is None:
> + layer = None
> + else:
> + layer = self.selected.layer
> + try:
> + selected = saved_layers.index(layer)
> + except ValueError:
> + selected = -1
> + return { 'selected' : selected,
> + 'loop': self.loop_mode,
> + 'animation': self.animation_mode,
> + 'layer_states' : layer_states,
> + 'ghost_before': self.ghost_before,
> + 'ghost_after': self.ghost_after }
This state should be part of the model instead, IMO.
The selected layer already is - it can be saved and restored in in lib/
directly. When loading an image, 'animation' can be set to True if there
are one or more layers marked as animation layers, so it does not need to be
explicitely saved.
> + def restore_state(self, state):
> [...]
> + if len(layer_states) != len(doc.layers):
> + print "layer_states out of sync with current document, not
> restoring"
> + else:
> + for (sticky, visible), layer in zip(layer_states,
> doc.layers):
> + layer.sticky = sticky
> + layer.visible = visible
Saving a custom XML attribute for each layer would certainly be more
ellegant. If the "sticky" attribute (which should probably called
"animated" instead) becomes part of the model, this is no problem. We
already have one for layer.visible... better just add one for layer.sticky
(or rather layer.animated or .is_animation_frame).
> + if self.animation_mode:
> + self.update_animation()
> + else:
> + self.reset_animation()
Since we cannot call those from lib/ there will have to be an observer for
this. (the existing canvas_observers can probably be used for that)
This would be a cleaner separation between model and GUI.
> def on_layer_del(self, button):
> doc = self.app.drawWindow.tdw.doc
> doc.remove_layer(layer=doc.get_current_layer())
> + if self.layers_list.animation_mode:
> + self.app.drawWindow.tdw.queue_draw()
Iik... looks like a hack... using canvas_observers should allow to get rid
of this (and other similar places).
> @@ -273,7 +274,7 @@ class Document():
> ext = ext.lower().replace('.', '')
> load = getattr(self, 'load_' + ext, self.unsupported)
> try:
> - load(filename)
> + view_settings = load(filename)
> except gobject.GError, e:
> traceback.print_exc()
> raise SaveLoadError, _('Error while loading: GError %s') % e
> @@ -283,6 +284,7 @@ class Document():
> self.command_stack.clear()
> self.unsaved_painting_time = 0.0
> self.call_doc_observers()
> + return view_settings
(See comment above about the design. I think you are drilling a small hole
into the existing concept it here ;-)
> [...]
> + keep_empty_layers = False
> + if options:
> + keep_empty_layers = options.get('keep_empty_layers', False)
> [..]
> - continue
> + if not keep_empty_layers:
> + continue
> opac = l.opacity
> x, y, w, h = l.surface.get_bbox()
> - pixbuf = l.surface.render_as_pixbuf()
> + if w == 0 or h == 0:
> + pixbuf = None
> + else:
> + pixbuf = l.surface.render_as_pixbuf()
I really hope we don't need all those "empty layer" special cases in the
saving code. I would prefer to just render a fixed spot of emptiness to the
special-casing here.
--
Martin Renold
_______________________________________________
Mypaint-discuss mailing list
[email protected]
https://mail.gna.org/listinfo/mypaint-discuss
