Bug fix: incorrect instance merging when unspecified-length arrays differed. (#656)

Because the altered data offset wasn't transferred from the instance
overrides to the instance's full symbol table, it was comparing the
default values rather than the actual values, so two instances that
should not have been merged, were merged.

Also fixed a (harmless) bug in ShadingSystemImpl::ShaderGroupBegin,
where the serialized group is parsed, where it was double-calling
Parameter() to add the parameters.

Fixes #655

Author: lgritz

CMakeLists.txt

@@ -543,7 +543,8 @@ TESTSUITE ( and-or-not-synonyms aastep arithmetic array array-derivs array-range
             layers layers-Ciassign layers-entry layers-lazy
             layers-nonlazycopy layers-repeatedoutputs
             linearstep
-            logic loop matrix message mergeinstances-nouserdata
+            logic loop matrix message
+            mergeinstances-nouserdata mergeinstances-vararray
             metadata-braces miscmath missing-shader
             noise noise-cell
             noise-gabor noise-gabor2d-filter noise-gabor3d-filter

src/liboslexec/instance.cpp

@@ -214,9 +214,13 @@ ShaderInstance::parameters (const ParamValueList &params)
 
     m_instoverrides.resize (std::max (0, lastparam()));
 
-    // Set the initial lockgeom on the instoverrides, based on the master.
-    for (int i = 0, e = (int)m_instoverrides.size(); i < e; ++i)
-        m_instoverrides[i].lockgeom (master()->symbol(i)->lockgeom());
+    // Set the initial lockgeom and dataoffset on the instoverrides, based
+    // on the master.
+    for (int i = 0, e = (int)m_instoverrides.size(); i < e; ++i) {
+        Symbol *sym = master()->symbol(i);
+        m_instoverrides[i].lockgeom (sym->lockgeom());
+        m_instoverrides[i].dataoffset (sym->dataoffset());
+    }
 
     BOOST_FOREACH (const ParamValue &p, params) {
         if (p.name().size() == 0)
@@ -257,6 +261,9 @@ ShaderInstance::parameters (const ParamValueList &params)
                              p.interp() == ParamValue::INTERP_CONSTANT);
             so->lockgeom (lockgeom);
 
+            DASSERT (so->dataoffset() == sm->dataoffset());
+            so->dataoffset (sm->dataoffset());
+
             if (paramtype.arraylen < 0) {
                 // An array of definite size was supplied to a parameter
                 // that was an array of indefinite size. Magic! The trick
@@ -460,6 +467,7 @@ ShaderInstance::copy_code_from_master (ShaderGroup &group)
                 si->valuesource (m_instoverrides[i].valuesource());
                 si->connected_down (m_instoverrides[i].connected_down());
                 si->lockgeom (m_instoverrides[i].lockgeom());
+                si->dataoffset (m_instoverrides[i].dataoffset());
                 si->data (param_storage(i));
             }
             if (shadingsys().is_renderer_output (layername(), si->name(), &group)) {

src/liboslexec/shadingsys.cpp

@@ -2117,7 +2117,6 @@ ShadingSystemImpl::ShaderGroupBegin (string_view groupname,
             // Zero-pad if we parsed fewer values than we needed
             intvals.resize (type.numelements()*type.aggregate, 0);
             ASSERT (int(type.numelements())*type.aggregate == int(intvals.size()));
-            Parameter (paramname, type, &intvals[0], lockgeom);
         } else if (type.basetype == TypeDesc::FLOAT) {
             floatvals.clear ();
             floatvals.reserve (vals_to_preallocate);
@@ -2137,7 +2136,6 @@ ShadingSystemImpl::ShaderGroupBegin (string_view groupname,
             // Zero-pad if we parsed fewer values than we needed
             floatvals.resize (type.numelements()*type.aggregate, 0);
             ASSERT (int(type.numelements())*type.aggregate == int(floatvals.size()));
-            Parameter (paramname, type, &floatvals[0], lockgeom);
         } else if (type.basetype == TypeDesc::STRING) {
             stringvals.clear ();
             stringvals.reserve (vals_to_preallocate);
@@ -2167,7 +2165,6 @@ ShadingSystemImpl::ShaderGroupBegin (string_view groupname,
             // Zero-pad if we parsed fewer values than we needed
             stringvals.resize (type.numelements()*type.aggregate, ustring());
             ASSERT (int(type.numelements())*type.aggregate == int(stringvals.size()));
-            Parameter (paramname, type, &stringvals[0], lockgeom);
         }
 
         if (Strutil::parse_prefix (p, "[[")) {  // hints
@@ -3177,5 +3174,3 @@ osl_bind_interpolated_param (void *sg_, const void *name, long long type,
     }
     return 0;  // no such user data
 }
-
-

testsuite/mergeinstances-vararray/arrayparam.osl

@@ -0,0 +1,5 @@
+shader arrayparam(
+	color in_value[] = { color(1) },
+	output color out_value = in_value[0] )
+{
+}

testsuite/mergeinstances-vararray/ref/out.txt

@@ -0,0 +1,4 @@
+Compiled arrayparam.osl -> arrayparam.oso
+Compiled simple.osl -> simple.oso
+1.000000 0.000000 1.000000
+

testsuite/mergeinstances-vararray/run.py

@@ -0,0 +1,7 @@
+#!/usr/bin/env python
+
+# This is a regression test for a bug in which instance merging was
+# done incorrectly for shaders that differed only in the values given
+# to parameters which were arrays of unspecified length.
+
+command = testshade("--group data/shadergroup")

testsuite/mergeinstances-vararray/shadergroup

@@ -0,0 +1,7 @@
+param color[1] in_value 0 0 1 ;
+shader arrayparam param_node_2 ;
+param color[1] in_value 1 0 0 ;
+shader arrayparam param_node_1 ;
+shader simple simple_surface ;
+connect param_node_1.out_value simple_surface.a ;
+connect param_node_2.out_value simple_surface.b ;

testsuite/mergeinstances-vararray/simple.osl

@@ -0,0 +1,7 @@
+surface simple(
+	color a = color(0),
+	color b = color(0) )
+{
+	printf("%f\n", a + b);
+	Ci = emission() * (a + b);
+}