1. The GLSLShader value cache was not working, because I forgot to save

   values in said cache.

2. So I created new memoize decorators skipUnchanged and Instanceify to
   implement the setter value cache.

3. And discovered that, for colour map changes, canvases were only being
   redrawn because the GLSLShader value cache was broken.

fsl/fsleyes/actions/__init__.py

@@ -300,6 +300,10 @@ class ActionFactory(object):
             @otherCustomAction(arg1=8)
             def myAction3(self):
                 # do things here
+
+
+    .. todo:: Merge/replace this class with the :class:`.memoize.Instanceify`
+              decorator.
     """
 
     

fsl/fsleyes/gl/glvolume.py

@@ -273,11 +273,14 @@ class GLVolume(globject.GLImageObject):
             if self.ready():
                 if fslgl.glvolume_funcs.updateShaderState(self):
                     self.notify()
+
+        def cmapUpdate(*a):
+            self.refreshColourTextures()
+            self.notify()
         
         def colourUpdate(*a):
             self.refreshColourTextures()
-            if self.ready():
-                shaderUpdate()
+            shaderUpdate()
 
         def imageRefresh(*a):
             async.wait([self.refreshImageTexture()], shaderUpdate)
@@ -314,8 +317,8 @@ class GLVolume(globject.GLImageObject):
         opts   .addListener('clippingRange',  lName, shaderUpdate,  weak=False)
         opts   .addListener('clipImage',      lName, clipUpdate,    weak=False)
         opts   .addListener('invertClipping', lName, shaderUpdate,  weak=False)
-        opts   .addListener('cmap',           lName, colourUpdate,  weak=False)
-        opts   .addListener('negativeCmap',   lName, colourUpdate,  weak=False)
+        opts   .addListener('cmap',           lName, cmapUpdate,    weak=False)
+        opts   .addListener('negativeCmap',   lName, cmapUpdate,    weak=False)
         opts   .addListener('useNegativeCmap',
                             lName, colourUpdate,  weak=False)
         opts   .addListener('invert',         lName, colourUpdate,  weak=False)

fsl/fsleyes/gl/shaders/arbp/program.py

@@ -18,6 +18,7 @@ import OpenGL.raw.GL._types           as gltypes
 import OpenGL.GL.ARB.fragment_program as arbfp
 import OpenGL.GL.ARB.vertex_program   as arbvp
 
+import fsl.utils.memoize              as memoize
 import                                   parse
 
 
@@ -207,12 +208,17 @@ class ARBPShader(object):
             gl.glDisableClientState(gl.GL_TEXTURE_COORD_ARRAY)
 
 
+    @memoize.Instanceify(memoize.skipUnchanged)
     def setVertParam(self, name, value):
         """Sets the value of the specified vertex program parameter.
 
         .. note:: It is assumed that the value is either a sequence of length
                   4 (for vector parameters), or a ``numpy`` array of shape 
                   ``(n, 4)`` (for matrix parameters).
+
+        .. note:: This method is decorated by the
+                  :func:`.memoize.skipUnchanged` decorator, which returns
+                  ``True`` if the value was changed, and ``False`` otherwise.
         """
 
         pos   = self.vertParamPositions[name]
@@ -225,10 +231,15 @@ class ARBPShader(object):
                 arbvp.GL_VERTEX_PROGRAM_ARB, pos + i,
                 row[0], row[1], row[2], row[3]) 
 
-    
+
+    @memoize.Instanceify(memoize.skipUnchanged)
     def setFragParam(self, name, value):
         """Sets the value of the specified vertex program parameter. See 
         :meth:`setVertParam` for infomration about possible values.
+
+        .. note:: This method is decorated by the
+                  :func:`.memoize.skipUnchanged` decorator, which returns
+                  ``True`` if the value was changed, and ``False`` otherwise.        
         """
         pos   = self.fragParamPositions[name]
         value = np.array(value, dtype=np.float32).reshape((-1, 4))

fsl/fsleyes/gl/shaders/glsl/program.py

@@ -18,6 +18,8 @@ import OpenGL.GL.ARB.instanced_arrays as arbia
 
 import parse
 
+import fsl.utils.memoize as memoize
+
 
 log = logging.getLogger(__name__)
 
@@ -149,12 +151,6 @@ class GLSLShader(object):
                                              self.vertUniforms,
                                              self.fragUniforms)
 
-        # We cache the most recent value for
-        # every uniform. When a call to set()
-        # is made, if the value is unchanged,
-        # we skip the GL call.
-        self.values = {n : None for n in self.positions.keys()}
-
         # Buffers for vertex attributes
         self.buffers = {}
 
@@ -239,31 +235,20 @@ class GLSLShader(object):
             gl.glDeleteBuffers(1, gltypes.GLuint(buf))
         self.program = None
         
-        
+
+    @memoize.Instanceify(memoize.skipUnchanged)
     def set(self, name, value):
         """Sets the value for the specified GLSL ``uniform`` variable.
 
         The ``GLSLShader`` keeps a copy of the value of every uniform, to
         avoid unnecessary GL calls.
 
-        :returns: ``True`` if the value was changed, ``False`` otherwise.
+        
+        .. note:: This method is decorated by the
+                  :func:`.memoize.skipUnchanged` decorator, which returns
+                  ``True`` if the value was changed, ``False`` otherwise.
         """
 
-        oldVal = self.values[name]
-
-        oldIsArray = isinstance(oldVal, np.ndarray)
-        newIsArray = isinstance(value,  np.ndarray)
-        isarray    = oldIsArray or newIsArray
-
-        if oldIsArray and (not newIsArray): value  = np.array(value)
-        if newIsArray and (not oldIsArray): oldVal = np.array(oldVal)
-
-        if isarray: nochange = np.all(oldVal == value)
-        else:       nochange =        oldVal == value
-
-        if nochange:
-            return False
-
         vPos  = self.positions[name]
         vType = self.types[    name]
 
@@ -278,8 +263,6 @@ class GLSLShader(object):
 
         setfunc(vPos, value)
 
-        return True
-
 
     def setAtt(self, name, value, divisor=None):
         """Sets the value for the specified GLSL ``attribute`` variable.

fsl/utils/memoize.py

@@ -10,10 +10,14 @@ a function:
  .. autosummary::
     :nosignatures:
 
+    Instanceify
     memoizeMD5
+    skipUnchanged
 """
 
+
 import hashlib
+import functools
 
 
 def memoizeMD5(func):
@@ -46,3 +50,130 @@ def memoizeMD5(func):
         return result
 
     return wrapper
+
+
+def skipUnchanged(func):
+    """This decorator is intended for use with *setter* functions - a function
+     which accepts a name and a value, and is intended to set some named
+     attribute to the given value.
+
+    This decorator keeps a cache of name-value pairs. When the decorator is
+    called with a specific name and value, the cache is checked and, if the
+    given value is the same as the cached value, the decorated function is
+    *not* called. If the given value is different from the cached value (or
+    there is no value), the decorated function is called.
+
+    .. note:: This decorator ignores the return value of the decorated
+              function.
+
+    :returns: ``True`` if the underlying setter function was called, ``False``
+              otherwise.
+    """
+
+    import numpy as np
+    
+    cache = {}
+    
+    def wrapper(name, value, *args, **kwargs):
+
+        oldVal = cache.get(name, None)
+
+        if oldVal is not None:
+            
+            oldIsArray = isinstance(oldVal, np.ndarray)
+            newIsArray = isinstance(value,  np.ndarray)
+            isarray    = oldIsArray or newIsArray
+
+            if isarray: nochange = np.all(oldVal == value)
+            else:       nochange =        oldVal == value
+
+            if nochange:
+                return False 
+
+        func(name, value, *args, **kwargs)
+
+        cache[name] = value
+
+        return True
+
+    return wrapper
+
+
+class Instanceify(object):
+    """This class is intended to be used to decorate other decorators, so they
+    can be applied to instance methods. For example, say we have the following
+    class::
+
+    
+        class Container(object):
+
+            def __init__(self):
+                self.__items = {}
+
+            @skipUnchanged
+            def set(self, name, value):
+                self.__items[name] = value
+
+    
+    Given this definition, a single :func:`skipUnchanged` decorator will be
+    created and shared amongst all ``Container`` instances. This is not ideal,
+    as the value cache created by the :func:`skipUnchanged` decorator should
+    be associated with a single ``Container`` instance.
+
+    
+    By redefining the ``Container`` class definition like so:
+
+    
+        class Container(object):
+
+            def __init__(self):
+                self.__items = {}
+
+            @Instanceify(skipUnchanged)
+            def set(self, name, value):
+                self.__items[name] = value
+
+
+    a separate :func:`skipUnchanged` decorator is created for, and associated
+    with, every ``Container`` instance.
+
+    
+    This is achieved because an ``Instanceify`` instance is a descriptor. When
+    first accessed as an instance attribute, an ``Instanceify`` instance will
+    create the real decorator function, and replace itself on the instance.
+    """
+
+    
+    def __init__(self, realDecorator):
+        """Create an ``Instanceify`` decorator.
+
+        :arg realDecorator: A reference to the decorator that is to be
+                            *instance-ified*.
+        """
+
+        self.__realDecorator = realDecorator
+        self.__func          = None
+
+
+    def __call__(self, func):
+        """Called immediately after :meth:`__init__`, and passed the method
+        that is to be decorated.
+        """
+        self.__func = func
+        return self
+
+
+    def __get__(self, instance, cls):
+        """When an ``Instanceify`` instance is accessed as an attribute of
+        another object, it will create the real (instance-ified) decorator,
+        and replace itself on the instance with the real decorator.
+        """
+
+        if instance is None:
+            return self.__func
+
+        method    = functools.partial(self.__func, instance)
+        decMethod = self.__realDecorator(method)
+
+        setattr(instance, self.__func.__name__, decMethod)
+        return functools.update_wrapper(decMethod, self.__func)