Material.clone() bug fix

Contribution Depot
1

Current version of Material.clone() shares MatParam.



Test code is below.

Test code misuse asset manager, but should work.

If clone() works well, we have 3 colored arrow.



import com.jme3.app.SimpleApplication;

import com.jme3.material.Material;

import com.jme3.math.ColorRGBA;

import com.jme3.math.Vector3f;

import com.jme3.scene.Geometry;

import com.jme3.scene.Node;

import com.jme3.scene.debug.Arrow;



public class TestAxisRods extends SimpleApplication {



@Override

public void simpleInitApp() {

AxisRods rods = new AxisRods(“axis”, 4);



rootNode.attachChild(rods);

}



public static void main(String[] args) {

TestAxisRods test = new TestAxisRods();

test.start();

}



public class AxisRods extends Node {

private Material material;



public AxisRods(String name, float width) {

super(name);

createArrow(“X”, Vector3f.UNIT_X, width, ColorRGBA.Red);

createArrow(“Y”, Vector3f.UNIT_Y, width, ColorRGBA.Green);

createArrow(“Z”, Vector3f.UNIT_Z, width, ColorRGBA.Blue);

}



private void createArrow(String name, Vector3f extent, float width, ColorRGBA color) {

if (material == null) {

material = new Material(assetManager, “Common/MatDefs/Misc/WireColor.j3md”);

}

Arrow arrow = new Arrow(extent);

Geometry geom = new Geometry(name, arrow);

arrow.setLineWidth(width); // make arrow thicker

attachChild(geom);

material.setColor(“m_Color”, color);

geom.setMaterial(material.clone());

}

}

}





Patch is below

Patch has generics(HashMap<>) but it is not showing because it has < > letter.

Doesn’t it matter?

And please check the button ‘xml’ and ‘patch’.

These buttons don’t work.

Other buttons use ‘insert_tag’ but these two buttons use ‘insert_tag_’



[patch]

Index: src/core/com/jme3/material/Material.java

===================================================================

— src/core/com/jme3/material/Material.java (revision 5849)

+++ src/core/com/jme3/material/Material.java (working copy)

@@ -28,6 +28,7 @@

import java.io.IOException;

import java.util.Collection;

import java.util.HashMap;

+import java.util.Map;

import java.util.logging.Level;

import java.util.logging.Logger;



@@ -129,7 +130,10 @@

}

mat.technique = null;

mat.techniques = new HashMap<String, Technique>();

  •        mat.paramValues = new HashMap<String, MatParam>(paramValues);<br />

  •        mat.paramValues = new HashMap<String, MatParam>();<br />

  •        for (Map.Entry<String, MatParam> entry : paramValues.entrySet()) {<br />

  •        	mat.paramValues.put(entry.getKey(), entry.getValue().clone());<br />

  •   	}<br />

return mat;

}catch (CloneNotSupportedException ex){

throw new AssertionError();



[/patch]

2

Cool, thanks. I will fix the buttons, for now simply write the tags by hand with square brackets: [pa tch] [/pa tch]

3

Look at the source code for HashMap [java 1.5/1.6].



mat.paramValues=new HashMap(paramValues);



is the same as:



mat.paramValues = new HashMap();

for (Map.Entry entry : paramValues.entrySet()) {

mat.paramValues.put(entry.getKey(), entry.getValue().clone());

}





Like all collections, when you pass the collection(or in this case a map) in the constructor it does an implicit copy. HashMap does the foreach & puts in the constructor.

4

No, my patch clones the MapParam instance.

It’s the difference.

5

@Lim, YongHoon



The issue is with your Test application.



Change:

[patch]

material.setColor(“m_Color”, color);

geom.setMaterial(material.clone());

[/patch]

to

[patch]

Material mat = material.clone();

mat.setColor(“m_Color”, color);

geom.setMaterial(mat);

[/patch]



I tested it with your AxixRods test and it works as expected.

6

I know it’s a workaround and I have tested it already.

Both code should work well because color is set before clone.

7

Well with spatials&nodes there is clone and deepClone. clone is always the cheap copy and deepClone is a clone of everything sans-mesh. I would argue that clone() stay how it is to prevent unnecessary object bloat. Clone first, then override with new matparam (mat.setColor() in this case). This way the object retains the cheap copy. Maybe add deepClone() for the extra functionality you want.



However, in general its better to operate on the cloned object.

8

How a patch is applied to the source?

Is it different from jme2?

I’d like to know the formal process of jme3 bug patch.

9

No, its the same as in jme2, just post your fix patch file in code/patch tags to the contributions forum.

Cheers,

Normen

10
hazmat said:
Well with spatials&nodes there is clone and deepClone. clone is always the cheap copy and deepClone is a clone of everything sans-mesh. I would argue that clone() stay how it is to prevent unnecessary object bloat. Clone first, then override with new matparam (mat.setColor() in this case). This way the object retains the cheap copy. Maybe add deepClone() for the extra functionality you want.

However, in general its better to operate on the cloned object.

Do we need deepClone() in Material class?
I guess that Spatial/Node needs deepClone() because spatial allows only one parent.
If a Material is needed more than once, we can just reuse the same object.
If it is right, deepClone() seems doesn't needed. It just makes user get confused.
11

deepClone() doesn’t actually do what you think it does for Spatials/Nodes.

It clones the mesh data in addition to the scene graph, while the regular clone will share the meshes.

I commited this patch, as well as another one, that replaces HashMap with ListMap.