From 58365c4b8a3929954718ac3e292ca77675dd5540 Mon Sep 17 00:00:00 2001 From: Stephen Gold Date: Tue, 17 Oct 2023 20:09:50 -0700 Subject: [PATCH] Increase MAX_DEFINES in DefineList (re-do) (#2116) * DefineList: re-implement using a BitSet to remove the size limit * TechniqueDef: DefineList size is no longer limited, so remove 2 checks * OpaqueComparatorTest: update expected sort order affected by DefineList * DefineListTest: rewrite tests to accept any valid hashCode() function --- .../java/com/jme3/material/TechniqueDef.java | 12 +-- .../main/java/com/jme3/shader/DefineList.java | 51 +++++++----- .../jme3/renderer/OpaqueComparatorTest.java | 10 +-- .../java/com/jme3/shader/DefineListTest.java | 81 ++++++++++++------- 4 files changed, 86 insertions(+), 68 deletions(-) diff --git a/jme3-core/src/main/java/com/jme3/material/TechniqueDef.java b/jme3-core/src/main/java/com/jme3/material/TechniqueDef.java index daa08d3266..b620c9ceaf 100644 --- a/jme3-core/src/main/java/com/jme3/material/TechniqueDef.java +++ b/jme3-core/src/main/java/com/jme3/material/TechniqueDef.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009-2021 jMonkeyEngine + * Copyright (c) 2009-2023 jMonkeyEngine * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -422,11 +422,6 @@ public VarType getDefineIdType(int defineId) { public void addShaderParamDefine(String paramName, VarType paramType, String defineName) { int defineId = defineNames.size(); - if (defineId >= DefineList.MAX_DEFINES) { - throw new IllegalStateException("Cannot have more than " + - DefineList.MAX_DEFINES + " defines on a technique."); - } - paramToDefineId.put(paramName, defineId); defineNames.add(defineName); defineTypes.add(paramType); @@ -445,11 +440,6 @@ public void addShaderParamDefine(String paramName, VarType paramType, String def public int addShaderUnmappedDefine(String defineName, VarType defineType) { int defineId = defineNames.size(); - if (defineId >= DefineList.MAX_DEFINES) { - throw new IllegalStateException("Cannot have more than " + - DefineList.MAX_DEFINES + " defines on a technique."); - } - defineNames.add(defineName); defineTypes.add(defineType); return defineId; diff --git a/jme3-core/src/main/java/com/jme3/shader/DefineList.java b/jme3-core/src/main/java/com/jme3/shader/DefineList.java index d368f24948..d4ee84fc51 100644 --- a/jme3-core/src/main/java/com/jme3/shader/DefineList.java +++ b/jme3-core/src/main/java/com/jme3/shader/DefineList.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009-2015 jMonkeyEngine + * Copyright (c) 2009-2023 jMonkeyEngine * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -32,6 +32,7 @@ package com.jme3.shader; import java.util.Arrays; +import java.util.BitSet; import java.util.List; /** @@ -41,20 +42,19 @@ */ public final class DefineList { - public static final int MAX_DEFINES = 64; - - private long isSet; + private final BitSet isSet; private final int[] values; public DefineList(int numValues) { - if (numValues < 0 || numValues > MAX_DEFINES) { - throw new IllegalArgumentException("numValues must be between 0 and 64"); + if (numValues < 0) { + throw new IllegalArgumentException("numValues must be >= 0"); } values = new int[numValues]; + isSet = new BitSet(numValues); } private DefineList(DefineList original) { - this.isSet = original.isSet; + this.isSet = (BitSet) original.isSet.clone(); this.values = new int[original.values.length]; System.arraycopy(original.values, 0, values, 0, values.length); } @@ -65,18 +65,18 @@ private void rangeCheck(int id) { public boolean isSet(int id) { rangeCheck(id); - return (isSet & (1L << id)) != 0; + return isSet.get(id); } public void unset(int id) { rangeCheck(id); - isSet &= ~(1L << id); + isSet.clear(id); values[id] = 0; } public void set(int id, int val) { rangeCheck(id); - isSet |= (1L << id); + isSet.set(id, true); values[id] = val; } @@ -124,7 +124,7 @@ public void setAll(DefineList other) { } public void clear() { - isSet = 0; + isSet.clear(); Arrays.fill(values, 0); } @@ -142,21 +142,30 @@ public int getInt(int id) { @Override public int hashCode() { - return (int) ((isSet >> 32) ^ isSet); + return isSet.hashCode(); } @Override - public boolean equals(Object other) { - DefineList o = (DefineList) other; - if (isSet == o.isSet) { - for (int i = 0; i < values.length; i++) { - if (values[i] != o.values[i]) { - return false; - } - } + public boolean equals(Object object) { + if (this == object) { return true; } - return false; + if (object == null || object.getClass() != getClass()) { + return false; + } + DefineList otherDefineList = (DefineList) object; + if (values.length != otherDefineList.values.length) { + return false; + } + if (!isSet.equals(otherDefineList.isSet)) { + return false; + } + for (int i = 0; i < values.length; i++) { + if (values[i] != otherDefineList.values[i]) { + return false; + } + } + return true; } public DefineList deepClone() { diff --git a/jme3-core/src/test/java/com/jme3/renderer/OpaqueComparatorTest.java b/jme3-core/src/test/java/com/jme3/renderer/OpaqueComparatorTest.java index c37df8389f..929281819e 100644 --- a/jme3-core/src/test/java/com/jme3/renderer/OpaqueComparatorTest.java +++ b/jme3-core/src/test/java/com/jme3/renderer/OpaqueComparatorTest.java @@ -252,8 +252,8 @@ public void testSortByShaderDefines() { lightingMatTCVColorLight.setBoolean("VertexLighting", true); lightingMatTCVColorLight.setBoolean("SeparateTexCoord", true); - testSort(lightingMat, lightingMatVColor, lightingMatVLight, - lightingMatVColorLight, lightingMatTC, lightingMatTCVColorLight); + testSort(lightingMatVColor, lightingMat, lightingMatVColorLight, + lightingMatVLight, lightingMatTC, lightingMatTCVColorLight); } @Test @@ -332,8 +332,8 @@ public void testSortByAll() { Material mat2000 = matBase2.clone(); mat2000.setName("2000"); - testSort(mat1100, mat1101, mat1102, mat1110, - mat1120, mat1121, mat1122, mat1140, - mat1200, mat1210, mat1220, mat2000); + testSort(mat1110, mat1100, mat1101, mat1102, + mat1140, mat1120, mat1121, mat1122, + mat1220, mat1210, mat1200, mat2000); } } diff --git a/jme3-core/src/test/java/com/jme3/shader/DefineListTest.java b/jme3-core/src/test/java/com/jme3/shader/DefineListTest.java index e2bc6b5cc5..ac3ada832b 100644 --- a/jme3-core/src/test/java/com/jme3/shader/DefineListTest.java +++ b/jme3-core/src/test/java/com/jme3/shader/DefineListTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009-2021 jMonkeyEngine + * Copyright (c) 2009-2023 jMonkeyEngine * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -91,7 +91,9 @@ private String generateSource(DefineList dl) { @Test public void testSourceInitial() { DefineList dl = new DefineList(NUM_DEFINES); - assert dl.hashCode() == 0; + for (int id = 0; id < NUM_DEFINES; ++id) { + assert !dl.isSet(id); + } assert generateSource(dl).equals(""); } @@ -100,19 +102,29 @@ public void testSourceBooleanDefine() { DefineList dl = new DefineList(NUM_DEFINES); dl.set(BOOL_VAR, true); - assert dl.hashCode() == 1; + for (int id = 0; id < NUM_DEFINES; ++id) { + boolean isBoolVar = (id == BOOL_VAR); + assert dl.isSet(id) == isBoolVar; + } assert generateSource(dl).equals("#define BOOL_VAR 1\n"); dl.set(BOOL_VAR, false); - assert dl.hashCode() == 0; + for (int id = 0; id < NUM_DEFINES; ++id) { + assert !dl.isSet(id); + } assert generateSource(dl).equals(""); dl.set(BOOL_VAR, true); - assert dl.hashCode() == 1; + for (int id = 0; id < NUM_DEFINES; ++id) { + boolean isBoolVar = (id == BOOL_VAR); + assert dl.isSet(id) == isBoolVar; + } assert generateSource(dl).equals("#define BOOL_VAR 1\n"); dl.unset(BOOL_VAR); - assert dl.hashCode() == 0; + for (int id = 0; id < NUM_DEFINES; ++id) { + assert !dl.isSet(id); + } assert generateSource(dl).equals(""); } @@ -120,26 +132,38 @@ public void testSourceBooleanDefine() { public void testSourceIntDefine() { DefineList dl = new DefineList(NUM_DEFINES); - int hashCodeWithInt = 1 << INT_VAR; - dl.set(INT_VAR, 123); - assert dl.hashCode() == hashCodeWithInt; + for (int id = 0; id < NUM_DEFINES; ++id) { + boolean isIntVar = (id == INT_VAR); + assert dl.isSet(id) == isIntVar; + } assert generateSource(dl).equals("#define INT_VAR 123\n"); dl.set(INT_VAR, 0); - assert dl.hashCode() == hashCodeWithInt; + for (int id = 0; id < NUM_DEFINES; ++id) { + boolean isIntVar = (id == INT_VAR); + assert dl.isSet(id) == isIntVar; + } assert generateSource(dl).equals("#define INT_VAR 0\n"); dl.set(INT_VAR, -99); - assert dl.hashCode() == hashCodeWithInt; + for (int id = 0; id < NUM_DEFINES; ++id) { + boolean isIntVar = (id == INT_VAR); + assert dl.isSet(id) == isIntVar; + } assert generateSource(dl).equals("#define INT_VAR -99\n"); dl.set(INT_VAR, Integer.MAX_VALUE); - assert dl.hashCode() == hashCodeWithInt; + for (int id = 0; id < NUM_DEFINES; ++id) { + boolean isIntVar = (id == INT_VAR); + assert dl.isSet(id) == isIntVar; + } assert generateSource(dl).equals("#define INT_VAR 2147483647\n"); dl.unset(INT_VAR); - assert dl.hashCode() == 0; + for (int id = 0; id < NUM_DEFINES; ++id) { + assert !dl.isSet(id); + } assert generateSource(dl).equals(""); } @@ -148,11 +172,17 @@ public void testSourceFloatDefine() { DefineList dl = new DefineList(NUM_DEFINES); dl.set(FLOAT_VAR, 1f); - assert dl.hashCode() == (1 << FLOAT_VAR); + for (int id = 0; id < NUM_DEFINES; ++id) { + boolean isFloatVar = (id == FLOAT_VAR); + assert dl.isSet(id) == isFloatVar; + } assert generateSource(dl).equals("#define FLOAT_VAR 1.0\n"); dl.set(FLOAT_VAR, 0f); - assert dl.hashCode() == (1 << FLOAT_VAR); + for (int id = 0; id < NUM_DEFINES; ++id) { + boolean isFloatVar = (id == FLOAT_VAR); + assert dl.isSet(id) == isFloatVar; + } assert generateSource(dl).equals("#define FLOAT_VAR 0.0\n"); dl.set(FLOAT_VAR, -1f); @@ -191,49 +221,38 @@ public void testEqualsAndHashCode() { DefineList dl1 = new DefineList(NUM_DEFINES); DefineList dl2 = new DefineList(NUM_DEFINES); - assertEquals(0, dl1.hashCode()); - assertEquals(0, dl2.hashCode()); + assertEquals(dl1.hashCode(), dl2.hashCode()); assertEquals(dl1, dl2); dl1.set(BOOL_VAR, true); - assertEquals(1, dl1.hashCode()); - assertEquals(0, dl2.hashCode()); assertNotEquals(dl1, dl2); dl2.set(BOOL_VAR, true); - assertEquals(1, dl1.hashCode()); - assertEquals(1, dl2.hashCode()); + assertEquals(dl1.hashCode(), dl2.hashCode()); assertEquals(dl1, dl2); dl1.set(INT_VAR, 2); - assertEquals(1 | 2, dl1.hashCode()); - assertEquals(1, dl2.hashCode()); assertNotEquals(dl1, dl2); dl2.set(INT_VAR, 2); - assertEquals(1 | 2, dl1.hashCode()); - assertEquals(1 | 2, dl2.hashCode()); + assertEquals(dl1.hashCode(), dl2.hashCode()); assertEquals(dl1, dl2); dl1.set(BOOL_VAR, false); - assertEquals(2, dl1.hashCode()); - assertEquals(1 | 2, dl2.hashCode()); assertNotEquals(dl1, dl2); dl2.unset(BOOL_VAR); - assertEquals(2, dl1.hashCode()); - assertEquals(2, dl2.hashCode()); + assertEquals(dl1.hashCode(), 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.hashCode(), dl2.hashCode()); assertEquals(dl1, dl2); }