Re: surface cdata property restricted to double or uint8

[John W. Eaton]

On 29-Aug-2010, Shai Ayal wrote:

| On Fri, Aug 27, 2010 at 7:02 PM, John W. Eaton <address@hidden> wrote:
| >
| > Why are the surface cdata and alphadata properties restricted to
| > double or uint8 arrays?  I see that this restriction is documented for
| > Matlab, but is there any reason we can't allow other data types for
| > these properties?  At the very least, why isn't single allowed?
| >
| > See also bug #30877.
| 
| I see no reason for the restriction, but relaxing it would have to be
| reflected in the convert_cdata function in graphics.cc
| I think that for the sake of optimization it accesses the underlaying
| c-array for cdata. However this means it has to do all the type
| conversions. How slower would it be if we access the cdata elements
| using octave's element operator, which will do type conversions for us
| auto-magically?
| 
| John, can you please have a look at this function?

Thanks for pointing out that this function would also need to be
changed.

Which element operator are you thinking of?
octave_value::fast_elem_extract?  That returns an octave_value object,
so that means for each element we would be constructing an
octave_value object and then immediately extracting a double value
from it.  That seems like a lot of overhead here.

How about something like the following change, which always treats cdata
as a double array internally?  That requires a data conversion to
double for the entire array, even though it is only really necessary
to convert one element at a time.  But at least it doesn't create a
temporary octave_value object for each array element.

If conversion of the entire array is undesirable, then maybe we could
use something like the second change below, which uses a template
function and can easily be extended to more data types.  For
simplicity, I think the first change is better.

Comments?

jwe

diff --git a/src/graphics.cc b/src/graphics.cc
--- a/src/graphics.cc
+++ b/src/graphics.cc
@@ -598,41 +598,38 @@
 
   double *av = a.fortran_vec ();
   const double *cmapv = cmap.data ();
-  const double *cv = 0;
-  const octave_uint8 *icv = 0;
-
-  if (cdata.is_integer_type ())
-    icv = cdata.uint8_array_value ().data ();
-  else
-    cv = cdata.array_value ().data ();
-
-  for (octave_idx_type i = 0; i < lda; i++)
-    {
-      double x = (cv ? cv[i] : double (icv[i]));
-
-      if (is_scaled)
-        x = xround ((nc - 1) * (x - clim(0)) / (clim(1) - clim(0)));
-      else
-        x = xround (x - 1);
-
-      if (xisnan (x))
+  const double *cv = cdata.array_value().data ();
+
+  if (! error_state)
+    {
+      for (octave_idx_type i = 0; i < lda; i++)
         {
-          av[i]       = x;
-          av[i+lda]   = x;
-          av[i+2*lda] = x;
-        }
-      else
-        {
-          if (x < 0)
-            x = 0;
-          else if (x >= nc)
-            x = (nc - 1);
-
-          octave_idx_type idx = static_cast<octave_idx_type> (x);
-
-          av[i]       = cmapv[idx];
-          av[i+lda]   = cmapv[idx+nc];
-          av[i+2*lda] = cmapv[idx+2*nc];
+          double x = cv[i];
+
+          if (is_scaled)
+            x = xround ((nc - 1) * (x - clim(0)) / (clim(1) - clim(0)));
+          else
+            x = xround (x - 1);
+
+          if (xisnan (x))
+            {
+              av[i]       = x;
+              av[i+lda]   = x;
+              av[i+2*lda] = x;
+            }
+          else
+            {
+              if (x < 0)
+                x = 0;
+              else if (x >= nc)
+                x = (nc - 1);
+
+              octave_idx_type idx = static_cast<octave_idx_type> (x);
+
+              av[i]       = cmapv[idx];
+              av[i+lda]   = cmapv[idx+nc];
+              av[i+2*lda] = cmapv[idx+2*nc];
+            }
         }
     }
 
diff --git a/src/graphics.h.in b/src/graphics.h.in
--- a/src/graphics.h.in
+++ b/src/graphics.h.in
@@ -3710,10 +3710,12 @@
         xdata.add_constraint (dim_vector (-1, -1));
         ydata.add_constraint (dim_vector (-1, -1));
         zdata.add_constraint (dim_vector (-1, -1));
+        alphadata.add_constraint ("single");
         alphadata.add_constraint ("double");
         alphadata.add_constraint ("uint8");
         alphadata.add_constraint (dim_vector (-1, -1));
         vertexnormals.add_constraint (dim_vector (-1, -1, 3));
+        cdata.add_constraint ("single");
         cdata.add_constraint ("double");
         cdata.add_constraint ("uint8");
         cdata.add_constraint (dim_vector (-1, -1));

diff --git a/src/graphics.cc b/src/graphics.cc
--- a/src/graphics.cc
+++ b/src/graphics.cc
@@ -552,6 +552,47 @@
  while (true);
 }
 
+static void
+convert_cdata_2 (bool is_scaled, double clim_0, double clim_1,
+                 const double *cmapv, double x, octave_idx_type lda,
+                 octave_idx_type nc, octave_idx_type i, double *av)
+{
+  if (is_scaled)
+    x = xround ((nc - 1) * (x - clim_0) / (clim_1 - clim_0));
+  else
+    x = xround (x - 1);
+
+  if (xisnan (x))
+    {
+      av[i]       = x;
+      av[i+lda]   = x;
+      av[i+2*lda] = x;
+    }
+  else
+    {
+      if (x < 0)
+        x = 0;
+      else if (x >= nc)
+        x = (nc - 1);
+
+      octave_idx_type idx = static_cast<octave_idx_type> (x);
+
+      av[i]       = cmapv[idx];
+      av[i+lda]   = cmapv[idx+nc];
+      av[i+2*lda] = cmapv[idx+2*nc];
+    }
+}
+
+template <class T>
+void
+convert_cdata_1 (bool is_scaled, double clim_0, double clim_1,
+                 const double *cmapv, const T *cv, octave_idx_type lda,
+                 octave_idx_type nc, double *av)
+{
+  for (octave_idx_type i = 0; i < lda; i++)
+    convert_cdata_2 (is_scaled, clim_0, clim_1, cmapv, cv[i], lda, nc, i, av);
+}
+
 static octave_value
 convert_cdata (const base_properties& props, const octave_value& cdata,
                bool is_scaled, int cdim)
@@ -598,43 +639,22 @@
 
   double *av = a.fortran_vec ();
   const double *cmapv = cmap.data ();
-  const double *cv = 0;
-  const octave_uint8 *icv = 0;
-
-  if (cdata.is_integer_type ())
-    icv = cdata.uint8_array_value ().data ();
+
+  double clim_0 = clim(0);
+  double clim_1 = clim(1);
+
+#define CONVERT_CDATA_1(T) \
+  convert_cdata_1 (is_scaled, clim_0, clim_1, cmapv, \
+                   cdata. T ## _value().data (), lda, nc, av)
+
+  if (cdata.is_uint8_type ())
+    CONVERT_CDATA_1 (uint8_array);
+  else if (cdata.is_single_type ())
+    CONVERT_CDATA_1 (float_array);
   else
-    cv = cdata.array_value ().data ();
-
-  for (octave_idx_type i = 0; i < lda; i++)
-    {
-      double x = (cv ? cv[i] : double (icv[i]));
-
-      if (is_scaled)
-        x = xround ((nc - 1) * (x - clim(0)) / (clim(1) - clim(0)));
-      else
-        x = xround (x - 1);
-
-      if (xisnan (x))
-        {
-          av[i]       = x;
-          av[i+lda]   = x;
-          av[i+2*lda] = x;
-        }
-      else
-        {
-          if (x < 0)
-            x = 0;
-          else if (x >= nc)
-            x = (nc - 1);
-
-          octave_idx_type idx = static_cast<octave_idx_type> (x);
-
-          av[i]       = cmapv[idx];
-          av[i+lda]   = cmapv[idx+nc];
-          av[i+2*lda] = cmapv[idx+2*nc];
-        }
-    }
+    CONVERT_CDATA_1 (array);
+
+#undef CONVERT_CDATA_1
 
   return octave_value (a);
 }
diff --git a/src/graphics.h.in b/src/graphics.h.in
--- a/src/graphics.h.in
+++ b/src/graphics.h.in
@@ -3710,10 +3710,12 @@
         xdata.add_constraint (dim_vector (-1, -1));
         ydata.add_constraint (dim_vector (-1, -1));
         zdata.add_constraint (dim_vector (-1, -1));
+        alphadata.add_constraint ("single");
         alphadata.add_constraint ("double");
         alphadata.add_constraint ("uint8");
         alphadata.add_constraint (dim_vector (-1, -1));
         vertexnormals.add_constraint (dim_vector (-1, -1, 3));
+        cdata.add_constraint ("single");
         cdata.add_constraint ("double");
         cdata.add_constraint ("uint8");
         cdata.add_constraint (dim_vector (-1, -1));