AssetManager vs BinaryImporter

Troubleshooting General Help
1

So I’m about to find a bug in the AssetManager it seems:
The case is this: I was about to implement MotionEvents (as Controls) into the SDK’s Scene Composer, where I noticed that after each Opening I have (only) the MotionEvent Controls duplicated.

After hours of searching through the SDK Code for a fault it turns out that this already happens when loading the j3o:
For some reason every serialized Control is loaded twice (i.e. the Node’s controlList contains 2 Controls with the same ID (means they are identical))

Since I know you like testcases I’ve put something together :stuck_out_tongue:

package mygame;

import com.jme3.app.SimpleApplication;
import com.jme3.export.binary.BinaryImporter;
import com.jme3.scene.Node;
import com.jme3.scene.Spatial;
import java.io.File;
import java.io.IOException;

/**
 * test
 * @author normenhansen
 */
public class Main extends SimpleApplication {

    public static void main(String[] args) throws IOException {
        Main app = new Main();
        app.start();
    }

    @Override
    public void simpleInitApp() {
        try {
            BinaryImporter importer = new BinaryImporter();
            Node node = (Node)importer.load(new File("test.j3o"));
            Spatial s = assetManager.loadModel("test.j3o");
            System.out.println(((Node)s).equals(node));
        } catch (Exception e) {}
    }

}

Now the question is: WHY?
What does the BinaryImporter do different? Not calling “cloneForSpatial” or something?

Note: This is on alpha-3 so it’s not related to the recent Cloning changes.
Btw: For this you have to put the test.j3o a) inside the asset root and b) in the project root (so …/ to assets)

2

Your file already contains the two controls so it’s not a valid test case.
The problem seems to be here:

3

Yeah you are right, but loading the thing leads to 4 controls then.
Thanks for the Find. I actually looked over MotionEvent’s Code but somehow I was too focused on the write()/read() stuff :slight_smile:

@nehon What do you think?
Should we completely remove the “spatial” from the Constructor and have people call .setSpatial() as always?
(But htat’s also unrelated from the Change in cloneForSpatial())

4

It’s weird that this constructor is called upon loading actually.
Laoding should initialize the MotionEvent with the empty constructor then set the control on the spatial

5

Yeah, it’s a small fault:

Your cloneForSpatial() accidentally is calling the wrong constructor.

Now I don’t know when the new Cloning will be used completely. If it’d be alpha-4 we wouldn’t need to really fix this at all.

6

You get 4 controls because spatials are cloned after being loaded via AssetManager, and cloning calls the buggy constructor again:

7

It’s funny… I just saw this code last night and was going to ask about it.

The new cloner will at least not have this problem because I used the no-arg constructor in the new jmeClone() method. :slight_smile:

8

That’s true.
Do you think you are done with the new cloning as of alpha-4?
And will the old Cloning stay at least for a while?

9

I’m trying to get the new cloning done for alpha-4 so that it can get tested properly. I’m doing my best to make sure it behaves the same (minus the things I’m sure are bugs)… but it’s going to be a lot of changes and it would be impossible to not have some bug.

I will leave the old clone methods for testing but the engine will start using the new cloner to do cloning for all of its internal cloning. And the Spatial.clone() methods will be converted to use it internally instead of cloneForSpatial().

The new approach simplifies a ton of code… and in the long road, simple = fewer bugs.

Edit: P.S.: if you are worried about some custom controls that you’ve written and their cloneForSpatial() method… 99% of the time you didn’t even need a cloneForSpatial() method in the first place. It’s need is rare and completely goes away with the new cloner.

1 Like
10

That’s the issue, here the empty constructor should be called.
I’ll fix it

11

Done

12

@pspeed
I wasn’t worried about my custom controls, I only wanted to see if it’s worth creating a PR/having it fixed.

@nehon
Thanks for that awesome fast fix :stuck_out_tongue:
One question though: Aren’t you missing a control.spatial = spatial; in CloneForSpatial?
(I’m no cloning expert, so I don’t know if setSpatial() is called afterwards, but I’ve seen some code explictly setting the spatial in cloneForSpatial)

13

small note, maybe useless.
I used to do a “new” in the clone method, but then the problem is that you cannot inherit easily the class, because you basically have to rewrite everything from the parent and add your stuff, and things in the parent could be private.
Not sure i am clear, so let’s take this as an example: if i extends MotionEvent i will have to override the cloneForSpatial (so it returns an object of my class, not a MotionEvent, the parent class). But then i will not have access to several fields marked private.

Solution: instead of using “new” in the parent cloneForSpatial, this parent should call “clone()” (the native method) because the clone method create a copy of the current object. So, if i want to extends MotionEvent i will only need to do
MyClass result = (MyClass) super.cloneForSpatial(spatial);
//my stuff here

hope it can help.