Merge pull request #572 from jMonkeyEngine/define_list_fix

Fix DefineList regression

jme3-core/src/main/java/com/jme3/shader/DefineList.java

@@ -43,30 +43,41 @@ public final class DefineList {
 
     public static final int MAX_DEFINES = 64;
 
-    private long hash;
+    private long isSet;
-    private final int[] vals;
+    private final int[] values;
 
     public DefineList(int numValues) {
         if (numValues < 0 || numValues > MAX_DEFINES) {
             throw new IllegalArgumentException("numValues must be between 0 and 64");
         }
-        vals = new int[numValues];
+        values = new int[numValues];
     }
 
     private DefineList(DefineList original) {
-        this.hash = original.hash;
+        this.isSet = original.isSet;
-        this.vals = new int[original.vals.length];
+        this.values = new int[original.values.length];
-        System.arraycopy(original.vals, 0, vals, 0, vals.length);
+        System.arraycopy(original.values, 0, values, 0, values.length);
+    }
+
+    private void rangeCheck(int id) {
+        assert 0 <= id && id < values.length;
+    }
+
+    public boolean isSet(int id) {
+        rangeCheck(id);
+        return (isSet & (1L << id)) != 0;
+    }
+
+    public void unset(int id) {
+        rangeCheck(id);
+        isSet &= ~(1L << id);
+        values[id] = 0;
     }
 
     public void set(int id, int val) {
-        assert 0 <= id && id < 64;
+        rangeCheck(id);
-        if (val != 0) {
+        isSet |= (1L << id);
-            hash |=  (1L << id);
+        values[id] = val;
-        } else {
-            hash &= ~(1L << id);
-        }
-        vals[id] = val;
     }
 
     public void set(int id, float val) {
@@ -74,15 +85,17 @@ public final class DefineList {
     }
 
     public void set(int id, boolean val) {
-        set(id, val ? 1 : 0);
+        if (val) {
+            set(id, 1);
+        } else {
+            // Because #ifdef usage is very common in shaders, unset the define
+            // instead of setting it to 0 for booleans.
+            unset(id);
+        }
     }
 
     public void set(int id, VarType type, Object value) {
-        if (value == null) {
+        if (value != null) {
-            set(id, 0);
-            return;
-        }
-
             switch (type) {
                 case Int:
                     set(id, (Integer) value);
@@ -97,44 +110,49 @@ public final class DefineList {
                     set(id, 1);
                     break;
             }
+        } else {
+            unset(id);
+        }
     }
 
     public void setAll(DefineList other) {
-        for (int i = 0; i < other.vals.length; i++) {
+        for (int i = 0; i < other.values.length; i++) {
-            if (other.vals[i] != 0) {
+            if (other.isSet(i)) {
-                vals[i] = other.vals[i];
+                set(i, other.getInt(i));
             }
         }
     }
 
     public void clear() {
-        hash = 0;
+        isSet = 0;
-        Arrays.fill(vals, 0);
+        Arrays.fill(values, 0);
     }
 
     public boolean getBoolean(int id) {
-        return vals[id] != 0;
+        return values[id] != 0;
     }
 
     public float getFloat(int id) {
-        return Float.intBitsToFloat(vals[id]);
+        return Float.intBitsToFloat(values[id]);
     }
 
     public int getInt(int id) {
-        return vals[id];
+        return values[id];
     }
 
     @Override
     public int hashCode() {
-        return (int)((hash >> 32) ^ hash);
+        return (int) ((isSet >> 32) ^ isSet);
     }
 
     @Override
     public boolean equals(Object other) {
         DefineList o = (DefineList) other;
-         if (hash == o.hash) {
+        if (isSet == o.isSet) {
-             for (int i = 0; i < vals.length; i++) {
+            for (int i = 0; i < values.length; i++) {
-                  if (vals[i] != o.vals[i]) return false;
+                if (values[i] != o.values[i]) {
+                    return false;
+                }
             }
             return true;
         }
@@ -146,16 +164,15 @@ public final class DefineList {
     }
 
     public void generateSource(StringBuilder sb, List<String> defineNames, List<VarType> defineTypes) {
-        for (int i = 0; i < vals.length; i++) {
+        for (int i = 0; i < values.length; i++) {
-            if (vals[i] != 0) {
+            if (!isSet(i)) {
-                String defineName = defineNames.get(i);
+                continue;
+            }
 
-                sb.append("#define ");
+            sb.append("#define ").append(defineNames.get(i)).append(' ');
-                sb.append(defineName);
-                sb.append(" ");
 
             if (defineTypes != null && defineTypes.get(i) == VarType.Float) {
-                    float val = Float.intBitsToFloat(vals[i]);
+                float val = Float.intBitsToFloat(values[i]);
                 if (Float.isInfinite(val) || Float.isNaN(val)) {
                     throw new IllegalArgumentException(
                             "GLSL does not support NaN "
@@ -163,11 +180,10 @@ public final class DefineList {
                 }
                 sb.append(val);
             } else {
-                    sb.append(vals[i]);
+                sb.append(values[i]);
             }
 
-                sb.append("\n");
+            sb.append('\n');
-            }
         }
     }
 

jme3-core/src/test/java/com/jme3/material/MaterialMatParamTest.java

@@ -54,16 +54,15 @@ import com.jme3.texture.Image.Format;
 import com.jme3.texture.Texture;
 import com.jme3.texture.Texture2D;
 import java.util.ArrayList;
-import java.util.HashMap;
+import static org.junit.Assert.assertEquals;
-import java.util.Map;
 import org.junit.Before;
 
 /**
- * Validates how {@link MatParamOverride MPOs} work on the material level.
+ * Validates {@link MatParam}s.
  *
  * @author Kirill Vainer
  */
-public class MaterialMatParamOverrideTest {
+public class MaterialMatParamTest {
 
     private static final HashSet<String> IGNORED_UNIFORMS = new HashSet<String>(
             Arrays.asList(new String[]{"m_ParallaxHeight", "m_Shininess", "m_BackfaceShadows"}));
@@ -84,6 +83,14 @@ public class MaterialMatParamOverrideTest {
         outUniforms(uniform("UseMaterialColors", VarType.Boolean, true));
     }
 
+    @Test
+    public void testBoolMpFalse() {
+        material("Common/MatDefs/Light/Lighting.j3md");
+        inputMp(mpoBool("UseMaterialColors", false));
+        outDefines(def("MATERIAL_COLORS", VarType.Boolean, false));
+        outUniforms(uniform("UseMaterialColors", VarType.Boolean, false));
+    }
+    
     @Test
     public void testBoolOverrideFalse() {
         material("Common/MatDefs/Light/Lighting.j3md");
@@ -118,6 +125,14 @@ public class MaterialMatParamOverrideTest {
         outUniforms(uniform("AlphaDiscardThreshold", VarType.Float, 3.12f));
     }
 
+    @Test
+    public void testFloatMpZero() {
+        material("Common/MatDefs/Light/Lighting.j3md");
+        inputMp(mpoFloat("AlphaDiscardThreshold", 0.0f));
+        outDefines(def("DISCARD_ALPHA", VarType.Float, 0.0f));
+        outUniforms(uniform("AlphaDiscardThreshold", VarType.Float, 0.0f));
+    }
+    
     @Test
     public void testFloatOverride() {
         material("Common/MatDefs/Light/Lighting.j3md");
@@ -191,6 +206,14 @@ public class MaterialMatParamOverrideTest {
         outUniforms(uniform("NumberOfBones", VarType.Int, 1234));
     }
     
+    @Test
+    public void testIntMpZero() {
+        material("Common/MatDefs/Light/Lighting.j3md");
+        inputMp(mpoInt("NumberOfBones", 0));
+        outDefines(def("NUM_BONES", VarType.Int, 0));
+        outUniforms(uniform("NumberOfBones", VarType.Int, 0));
+    }
+
     @Test
     public void testIntOverride() {
         material("Common/MatDefs/Light/Lighting.j3md");
@@ -377,25 +400,31 @@ public class MaterialMatParamOverrideTest {
         outTextures(tex1);
     }
 
-    private static final class Define {
+    private static class Define {
 
         public String name;
         public VarType type;
         public Object value;
 
         @Override
-        public int hashCode() {
+        public String toString() {
-            int hash = 3;
+            switch (type) {
-            hash = 89 * hash + this.name.hashCode();
+                case Boolean:
-            hash = 89 * hash + this.type.hashCode();
+                    if ((Boolean)value) {
-            hash = 89 * hash + this.value.hashCode();
+                        return "#define " + name + " 1\n";
-            return hash;
+                    } else {
+                        return "";
+                    }
+                case Int:
+                case Float:
+                    return "#define " + name + " " + value + "\n";
+                default:
+                    if (value != null) {
+                        return "#define " + name + " 1\n";
+                    } else {
+                        return "";
+                    }
             }
-
-        @Override
-        public boolean equals(Object obj) {
-            final Define other = (Define) obj;
-            return this.name.equals(other.name) && this.type.equals(other.type) && this.value.equals(other.value);
         }
     }
 
@@ -411,13 +440,13 @@ public class MaterialMatParamOverrideTest {
     private final NullRenderer renderer = new NullRenderer() {
         @Override
         public void setShader(Shader shader) {
-            MaterialMatParamOverrideTest.this.usedShader = shader;
+            MaterialMatParamTest.this.usedShader = shader;
             evaluated = true;
         }
 
         @Override
         public void setTexture(int unit, Texture texture) {
-            MaterialMatParamOverrideTest.this.usedTextures[unit] = texture;
+            MaterialMatParamTest.this.usedTextures[unit] = texture;
         }
     };
     private final RenderManager renderManager = new RenderManager(renderer);
@@ -512,9 +541,9 @@ public class MaterialMatParamOverrideTest {
     }
 
     private void outDefines(Define... expectedDefinesArray) {
-        Map<String, Define> nameToDefineMap = new HashMap<String, Define>();
+        StringBuilder expectedDefineSource = new StringBuilder();
         for (Define define : expectedDefinesArray) {
-            nameToDefineMap.put(define.name, define);
+            expectedDefineSource.append(define.toString());
         }
         
         if (!evaluated) {
@@ -525,56 +554,11 @@ public class MaterialMatParamOverrideTest {
         Technique tech = mat.getActiveTechnique();
         TechniqueDef def = tech.getDef();
         DefineList actualDefines = tech.getDynamicDefines();
-
         String[] defineNames = def.getDefineNames();
         VarType[] defineTypes = def.getDefineTypes();
+        String actualDefineSource = actualDefines.generateSource(Arrays.asList(defineNames), Arrays.asList(defineTypes));
 
-        Assert.assertEquals(defineNames.length, defineTypes.length);
+        assertEquals(expectedDefineSource.toString(), actualDefineSource);
-
-        for (int index = 0; index < defineNames.length; index++) {
-            String name = defineNames[index];
-            VarType type = defineTypes[index];
-            Define expectedDefine = nameToDefineMap.remove(name);
-            Object expectedValue = null;
-
-            if (expectedDefine != null) {
-                Assert.assertEquals(expectedDefine.type, type);
-                expectedValue = expectedDefine.value;
-            }
-
-            switch (type) {
-                case Boolean:
-                    if (expectedValue != null) {
-                        Assert.assertEquals((boolean) (Boolean) expectedValue, actualDefines.getBoolean(index));
-                    } else {
-                        Assert.assertEquals(false, actualDefines.getBoolean(index));
-                    }
-                    break;
-                case Int:
-                    if (expectedValue != null) {
-                        Assert.assertEquals((int) (Integer) expectedValue, actualDefines.getInt(index));
-                    } else {
-                        Assert.assertEquals(0, actualDefines.getInt(index));
-                    }
-                    break;
-                case Float:
-                    if (expectedValue != null) {
-                        Assert.assertEquals((float) (Float) expectedValue, actualDefines.getFloat(index), 0f);
-                    } else {
-                        Assert.assertEquals(0f, actualDefines.getFloat(index), 0f);
-                    }
-                    break;
-                default:
-                    if (expectedValue != null) {
-                        Assert.assertEquals(1, actualDefines.getInt(index));
-                    } else {
-                        Assert.assertEquals(0, actualDefines.getInt(index));
-                    }
-                    break;
-            }
-        }
-
-        Assert.assertTrue(nameToDefineMap.isEmpty());
     }
 
     private void outUniforms(Uniform... uniforms) {

jme3-core/src/test/java/com/jme3/shader/DefineListTest.java

@@ -107,6 +107,14 @@ public class DefineListTest {
         dl.set(BOOL_VAR, false);
         assert dl.hashCode() == 0;
         assert generateSource(dl).equals("");
+
+        dl.set(BOOL_VAR, true);
+        assert dl.hashCode() == 1;
+        assert generateSource(dl).equals("#define BOOL_VAR 1\n");
+
+        dl.unset(BOOL_VAR);
+        assert dl.hashCode() == 0;
+        assert generateSource(dl).equals("");
     }
 
     @Test
@@ -120,8 +128,8 @@ public class DefineListTest {
         assert generateSource(dl).equals("#define INT_VAR 123\n");
 
         dl.set(INT_VAR, 0);
-        assert dl.hashCode() == 0;
+        assert dl.hashCode() == hashCodeWithInt;
-        assert generateSource(dl).equals("");
+        assert generateSource(dl).equals("#define INT_VAR 0\n");
 
         dl.set(INT_VAR, -99);
         assert dl.hashCode() == hashCodeWithInt;
@@ -130,6 +138,10 @@ public class DefineListTest {
         dl.set(INT_VAR, Integer.MAX_VALUE);
         assert dl.hashCode() == hashCodeWithInt;
         assert generateSource(dl).equals("#define INT_VAR 2147483647\n");
+
+        dl.unset(INT_VAR);
+        assert dl.hashCode() == 0;
+        assert generateSource(dl).equals("");
     }
 
     @Test
@@ -141,8 +153,8 @@ public class DefineListTest {
         assert generateSource(dl).equals("#define FLOAT_VAR 1.0\n");
 
         dl.set(FLOAT_VAR, 0f);
-        assert dl.hashCode() == 0;
+        assert dl.hashCode() == (1 << FLOAT_VAR);
-        assert generateSource(dl).equals("");
+        assert generateSource(dl).equals("#define FLOAT_VAR 0.0\n");
 
         dl.set(FLOAT_VAR, -1f);
         assert generateSource(dl).equals("#define FLOAT_VAR -1.0\n");
@@ -157,19 +169,22 @@ public class DefineListTest {
             dl.set(FLOAT_VAR, Float.NaN);
             generateSource(dl);
             assert false;
-        } catch (IllegalArgumentException ex) { }
+        } catch (IllegalArgumentException ex) {
+        }
 
         try {
             dl.set(FLOAT_VAR, Float.POSITIVE_INFINITY);
             generateSource(dl);
             assert false;
-        } catch (IllegalArgumentException ex) { }
+        } catch (IllegalArgumentException ex) {
+        }
 
         try {
             dl.set(FLOAT_VAR, Float.NEGATIVE_INFINITY);
             generateSource(dl);
             assert false;
-        } catch (IllegalArgumentException ex) { }
+        } catch (IllegalArgumentException ex) {
+        }
     }
 
     @Test
@@ -177,31 +192,50 @@ public class DefineListTest {
         DefineList dl1 = new DefineList(NUM_DEFINES);
         DefineList dl2 = new DefineList(NUM_DEFINES);
 
-        assertTrue(dl1.hashCode() == 0);
+        assertEquals(0, dl1.hashCode());
+        assertEquals(0, dl2.hashCode());
         assertEquals(dl1, dl2);
 
         dl1.set(BOOL_VAR, true);
 
-        assertTrue(dl1.hashCode() == 1);
+        assertEquals(1, dl1.hashCode());
-        assertNotSame(dl1, dl2);
+        assertEquals(0, dl2.hashCode());
+        assertNotEquals(dl1, dl2);
 
         dl2.set(BOOL_VAR, true);
 
+        assertEquals(1, dl1.hashCode());
+        assertEquals(1, dl2.hashCode());
         assertEquals(dl1, dl2);
 
         dl1.set(INT_VAR, 2);
 
-        assertTrue(dl1.hashCode() == (1|2));
+        assertEquals(1 | 2, dl1.hashCode());
-        assertNotSame(dl1, dl2);
+        assertEquals(1, dl2.hashCode());
+        assertNotEquals(dl1, dl2);
 
         dl2.set(INT_VAR, 2);
 
+        assertEquals(1 | 2, dl1.hashCode());
+        assertEquals(1 | 2, dl2.hashCode());
         assertEquals(dl1, dl2);
 
         dl1.set(BOOL_VAR, false);
 
-        assertTrue(dl1.hashCode() == 2);
+        assertEquals(2, dl1.hashCode());
-        assertNotSame(dl1, dl2);
+        assertEquals(1 | 2, dl2.hashCode());
+        assertNotEquals(dl1, dl2);
+
+        dl2.unset(BOOL_VAR);
+
+        assertEquals(2, dl1.hashCode());
+        assertEquals(2, dl2.hashCode());
+        assertEquals(dl1, dl2); // unset is the same as false
+
+        dl1.unset(BOOL_VAR);
+        assertEquals(2, dl1.hashCode());
+        assertEquals(2, dl2.hashCode());
+        assertEquals(dl1, dl2);
     }
 
     @Test
@@ -209,25 +243,31 @@ public class DefineListTest {
         DefineList dl1 = new DefineList(NUM_DEFINES);
         DefineList dl2 = dl1.deepClone();
 
-        assertFalse(dl1 == dl2);
+        assertNotSame(dl1, dl2);
-        assertTrue(dl1.equals(dl2));
+        assertEquals(dl1, dl2);
-        assertTrue(dl1.hashCode() == dl2.hashCode());
+        assertEquals(dl1.hashCode(), dl2.hashCode());
 
         dl1.set(BOOL_VAR, true);
         dl2 = dl1.deepClone();
 
-        assertTrue(dl1.equals(dl2));
+        assertEquals(dl1, dl2);
-        assertTrue(dl1.hashCode() == dl2.hashCode());
+        assertEquals(dl1.hashCode(), dl2.hashCode());
+
+        dl1.set(BOOL_VAR, false);
+        dl2 = dl1.deepClone();
+
+        assertEquals(dl1, dl2);
+        assertEquals(dl1.hashCode(), dl2.hashCode());
 
         dl1.set(INT_VAR, 123);
 
-        assertFalse(dl1.equals(dl2));
+        assertNotEquals(dl1, dl2);
-        assertFalse(dl1.hashCode() == dl2.hashCode());
+        assertNotEquals(dl1.hashCode(), dl2.hashCode());
 
         dl2 = dl1.deepClone();
 
-        assertTrue(dl1.equals(dl2));
+        assertEquals(dl1, dl2);
-        assertTrue(dl1.hashCode() == dl2.hashCode());
+        assertEquals(dl1.hashCode(), dl2.hashCode());
     }
 
     @Test
@@ -242,8 +282,8 @@ public class DefineListTest {
 
         dl.set(INT_VAR, 123);
 
-        assertEquals("#define BOOL_VAR 1\n" + 
+        assertEquals("#define BOOL_VAR 1\n"
-                     "#define INT_VAR 123\n", generateSource(dl));
+                + "#define INT_VAR 123\n", generateSource(dl));
 
         dl.set(BOOL_VAR, false);
 
@@ -252,15 +292,18 @@ public class DefineListTest {
         dl.set(BOOL_VAR, true);
 
         // should have predictable ordering based on defineId
-        assertEquals("#define BOOL_VAR 1\n" + 
+        assertEquals("#define BOOL_VAR 1\n"
-                     "#define INT_VAR 123\n", generateSource(dl));
+                + "#define INT_VAR 123\n", generateSource(dl));
+
+        dl.unset(BOOL_VAR);
+        assertEquals("#define INT_VAR 123\n", generateSource(dl));
     }
 
-    private static String doLookup(HashMap<DefineList, String> map, boolean boolVal, int intVal, float floatVal) {
+    private static String doLookup(HashMap<DefineList, String> map, Boolean boolVal, Integer intVal, Float floatVal) {
         DefineList dl = new DefineList(NUM_DEFINES);
-        dl.set(BOOL_VAR, boolVal);
+        dl.set(BOOL_VAR, VarType.Boolean, boolVal);
-        dl.set(INT_VAR, intVal);
+        dl.set(INT_VAR, VarType.Int, intVal);
-        dl.set(FLOAT_VAR, floatVal);
+        dl.set(FLOAT_VAR, VarType.Float, floatVal);
         return map.get(dl);
     }
 
@@ -272,7 +315,7 @@ public class DefineListTest {
         String STR_BOOL_INT = "This define list has both a boolean and int value";
         String STR_BOOL_INT_FLOAT = "This define list has a boolean, int, and float value";
 
-        HashMap<DefineList, String> map = new HashMap<DefineList, String>();
+        HashMap<DefineList, String> map = new HashMap<>();
 
         DefineList lookup = new DefineList(NUM_DEFINES);
 
@@ -291,10 +334,10 @@ public class DefineListTest {
         lookup.set(FLOAT_VAR, FastMath.PI);
         map.put(lookup.deepClone(), STR_BOOL_INT_FLOAT);
 
-        assertEquals(doLookup(map, false, 0, 0f), STR_EMPTY);
+        assertEquals(STR_EMPTY, doLookup(map, null, null, null));
-        assertEquals(doLookup(map, false, 123, 0f), STR_INT);
+        assertEquals(STR_INT, doLookup(map, false, 123, null));
-        assertEquals(doLookup(map, true, 0, 0f), STR_BOOL);
+        assertEquals(STR_BOOL, doLookup(map, true, null, null));
-        assertEquals(doLookup(map, true, 123, 0f), STR_BOOL_INT);
+        assertEquals(STR_BOOL_INT, doLookup(map, true, 123, null));
-        assertEquals(doLookup(map, true, 123, FastMath.PI), STR_BOOL_INT_FLOAT);
+        assertEquals(STR_BOOL_INT_FLOAT, doLookup(map, true, 123, FastMath.PI));
     }
 }