More weirdness when cloning BitmapText

Hi!

Thanks @pspeed for the rapid fixing of the issue here: Exception on BitmapText.clone() · Issue #577 · jMonkeyEngine/jmonkeyengine · GitHub
This solves the crashing and I can confirm it was no regression bug. BitmapText.clone() always did unexpected things.

In my game I use cloning for a drag and drop system in the GUI. As you can see below, changes in the original text (greyed out icon) are reflected by the clone (item anchored to mouse cursor). This is how it always was and I thought it was kinda neat in this particular use case but it’s not what I would expect.

So I did some testing and noticed more weirdness regarding the cloning of BitmapText.
I hope the comments in the different test cases below illustrate the issues.

import com.jme3.app.SimpleApplication;
import com.jme3.font.BitmapFont;
import com.jme3.font.BitmapText;
import com.jme3.scene.Node;

public class CloneTest3 extends SimpleApplication {
    public CloneTest3() { super(null); }

    private BitmapText mkText(String str) {
        BitmapFont font = assetManager.loadFont("Interface/Fonts/Default.fnt");
        BitmapText text = new BitmapText(font);
        text.setText(str);
        text.setLocalTranslation(300, 200, 0);
        return text;
    }

    // Works, displays text
    private void case1() {
        BitmapText text = mkText("case1");
        guiNode.attachChild(text);
    }

    // Displays nothing
    private void case2() {
        BitmapText orig = mkText("case2");
        guiNode.attachChild(orig);

        BitmapText copy = orig.clone(); // This makes the text disappear
    }

    // Cloning works when BitmapText ist nested inside a Node
    private void case3() {
        BitmapText orig = mkText("case3");

        Node node = new Node();
        node.attachChild(orig);
        guiNode.attachChild(node);

        Node nodeCopy = (Node) node.clone();
        // Everything still visible
    }

    // Cloning Node with nested BitmapText doesn't make deep copy?
    private void case4() {
        BitmapText orig = mkText("case4");

        Node node = new Node();
        node.attachChild(orig);
        guiNode.attachChild(node);

        Node nodeCopy = (Node) node.clone();
        nodeCopy.move(0, -30, 0);
        guiNode.attachChild(nodeCopy);

        // Changes displayed text of copy AND of original
        BitmapText textCopy = (BitmapText) nodeCopy.getChild(0);
        textCopy.setText("new text for copy (case4)");

        // The values returned by getText() are as expected
        System.out.println("orig text: " + orig.getText());      // Outputs "case4"
        System.out.println("copy text: " + textCopy.getText());  // Outputs "new text..."
    }

    // Setting new text in the original after it has been cloned doesn't work
    private void case5() {
        BitmapText orig = mkText("case5");

        Node node = new Node();
        node.attachChild(orig);
        guiNode.attachChild(node);

        Node nodeCopy = (Node) node.clone();
        nodeCopy.move(0, -30, 0);
        guiNode.attachChild(nodeCopy);

        // Doesn't change displayed text
        // This is strange since it works similarly in my game.
        // There, changes to orig however are reflected in the cloned BitmapText.
        orig.setText("new text for original (case5)");

        // Again, the value returned by getText() is as expected
        System.out.println("orig text: " + orig.getText()); // Outputs "new text..."
    }

    public void simpleInitApp() {
        case5();
    }

    public static void main(String[] args) {
        CloneTest3 app = new CloneTest3();
        app.setShowSettings(false);
        app.start();
    }
}

I guess something’s still wrong with deep copying a BitmapText. Can’t help any further though since atm I neither understand the Font things nor the Cloner.

Using latest version from master branch (157af24) and Java 1.8.0_102.

I’ve committed a fix to master that may fix this. The Letters object was getting the old string block instead of the cloned one. On code review, that’s the only ‘leak’ I saw.

Didn’t change the output of my test cases. :frowning: How is this.block referring to something else than block?
Also, isn’t clone.block = block.clone(); on BitmapText line 83 doing the clone twice in the end? (same with text pages)

Yes, you are right. I was thinking of the more typical Java cloning pattern. We are already only dealing with local fields.

Could be… BitmapText’s clone() method is not right… it should be calling the JME cloner. I never bothered to do BitmapText’s cloning properly because it was all jacked up to begin with I guess.

I’ll take a closer look.

I’ve checked in a fix to clone() but I don’t think it will really fix the issue.

You can try it.

That fixed case 2 :slight_smile:

I’ve committed another fix… this one is promising but I’m no longer confident enough to say for sure it will fix anything. :slight_smile:

The textPages array was never being cloned… so presumably it was being shared from one clone to the next. It was even final so… yeah.

Check to see if this fixes the last of the issues.

Thanks, I too had another go at it and I think it has to do with the other stuff from parent classes that are cloned with it.

Cloning the BitmapText which extends Node also clones the children which then stay attached to the copy.

BitmapTextPage (extends Geometry) does a deep copy of the mesh in the clone() method but this is not called since the Cloner prefers Spatial.jmeClone() and Geometry.cloneFields().

The code below for BitmapText.cloneFields() seems to work with my simple test cases and fixes the bug in my game.

public void cloneFields( Cloner cloner, Object original ) {
    super.cloneFields(cloner, original);

    detachAllChildren();
    textPages = textPages.clone();
    for( int i = 0; i < textPages.length; i++ ) {
        // Doesn't call BitmapTextPage.clone() which does a deep copy of the Geometry.
        // Cloner calls Spatial.jmeClone() and Geometry.cloneFields() instead.
        //textPages[i] = cloner.clone(textPages[i]);

        textPages[i] = textPages[i].clone();
        attachChild(textPages[i]);
    }
    
    this.block = block != null ? block.clone() : null;

    this.letters = new Letters(font, block, letters.getQuad().isRightToLeft());
}

Of course it would be much nicer to use a BitmapTextPage.cloneFields() method and use the already existing Cloner instance.
Can the Cloner ignore the children of BitmapText so we don’t have to call detachAllChildren()?


Also added this to StringBlock.clone()

        if (tabPos != null)
            clone.tabPos = tabPos.clone();

The whole point of the cloner is that you shouldn’t have to do that BS because it automatically fixes up shared references.

So it would better to fix cloning rather than hack around it.

I’m checked in another fix that should fix the cloning of BitmapTextPage. But who knows?

Aah I get it! The Cloner keeps track of all the cloned objects so it can replace all references in the whole cloned tree. Of course, that solves the problem of the cloned children. Very nice!

Your latest commit does the trick.

I still think tabPos needs to be manually cloned in StringBlock.clone().

Probably. When I rewrote all of the cloning it was really a lot of work and it was really hard not to try to fix everything.

Feel free to submit a patch for that one.

Edit: I mean if you want to. And please submit it to jme3.1 branch if you do. Thanks. :slight_smile:

Will do - tomorrow :grimacing: Thank you!