Spatial's ID Switch

Ok, back to working on this. Here is my plan:



Here's my plan:


  1. When a user creates a new spatial with: public Spatial(String name) the Id field (switched to int) is set to:

            id = name.toHashMap();
  2. Add a constructor like: public Spatial(int name)

            id = name;
  3. Think more about how we might want to store the actual string variation.



    llama, I know we talked about the WeakHashMap and turning them on with the JVM flag. However, it seems to me that there might be valid times to keep the name a string for only some nodes. A quick and dirty solution might be to keep String name = null in the Spatial, add a boolean to the constructor that takes the String, if the boolean is true set name to the String (as well as the int) otherwise leave the string null.



    You guys know more about the intricacies of the JVM than I do, while this solution is the easiest, is declaring a null string still taking memory (does the JVM allocate space for the possible addition on the string?).



    Any other ideas to solve the same thing? I.e. some nodes might have a string set for full descriptive purposes.



    I suppose I am a little torn. Do we want to make the String representation purely debug only, or do we want to allow the user to pick and choose? I'm open for debate on this, as the only thing I am sure on is switching to the int right now, and that will take me an hour or two.


Well, remember. If we're using the hashCode() of String to init the ID field, even with debugging turned off you can still use (for example) Node's getChild(String). Node will just use the hashcode to look for it instead fo the String.



What will be affected is giving an object a name, and showing it to the user (like in the picking demo) or doing someone with the actual name like some function in TerrainPage seem to do (you shouldn't be storing that kind of data in a String anyway, IMHO).



Personally I'd rather break things for them than for the rest. If you want to keep "naming" a core part of the API everyone who wants to use your optimization will have to change constructors, and for doing that they'll loose debugging. If you remove it as a "core" feature, people who currently give object a name will have to change their code. But remember: their work will still work for now, they just need to add the flag till they fix things.



Perhaps we can help them porting by introducing some methods like setObjectName() and getObjectName() that use the WeakHashMap (or even a String field… I don't care so much about the 4 bytes, more about keeping the current API) regardless of wether there is a flag or not.

Like Llama I think keeping the old API would be good. Storing the Strings in a field would be ok for me (a WeakHashMap, too, to save the 4 bytes). And add some flag that switches off names. This should result in an exception if methods which use names are called.

Using the hash value of a String to identify childs could lead to very hard to track error - we should not do that.



Why do you want to let the int id be specified? Just count them up - we can use the trick for adding spatials to nodes then:

> When spatials get ids we can do a little trick here - stick with the ArrayList (as

> it's cheap regarding memory usage and iteration) but additionally store the

> maximum id in the list and check if an added Spatial has a greater id (if so, don't

> search for it in the list). This should help alot as in most cases Spatials are added

> in creation order (this surely does work only if ids are counted up).

Ok, let's not worry about keeping a string representation for the moment.



Working off Irrisor's idea.


Spatial {

irrisor said:

Using the hash value of a String to identify childs could lead to _very_ hard to track error - we should not do that.


Are you talking about hash collisions here? Well I suppose it's a theoretical possibility. That would mean giving up on using "names" to get spatials from a Node. I guess you're right though, the chance is small but it's still there.


Why do you want to let the int id be specified? Just count them up - we can use the trick for adding spatials to nodes then:
> When spatials get ids we can do a little trick here - stick with the ArrayList (as
> it's cheap regarding memory usage and iteration) but additionally store the
> maximum id in the list and check if an added Spatial has a greater id (if so, don't
> search for it in the list). This should help alot as in most cases Spatials are added
> in creation order (this surely does work only if ids are counted up).


You're talking about the part where jME checks to see if a Spatial is already added to a Node? I think that should be be changed to give a warning when you're in debug mode. Is there any reason we should allow it in the first place?
llama said:

Are you talking about hash collisions here?

Yes
That would mean giving up on using "names" to get spatials from a Node.

You can still use the names or int ids, but I never thought it makes sense (neither with names nor ids).

You're talking about the part where jME checks to see if a Spatial is already added to a Node?

Yes
I think that should be be changed to give a warning when you're in debug mode. Is there any reason we should allow it in the first place?

Maybe I did not give enough context here: Adding many objects to a Node is very slow at this time. If we would store the maximum id one could check if the id of a newly added object is greater than the stored one. If true the object cannot be in the list and we can safely add it. So it does not have to do with debugging but with optimization.
irrisor said:

llama said:

I think that should be be changed to give a warning when you're in debug mode. Is there any reason we should allow it in the first place?

Maybe I did not give enough context here: Adding many objects to a Node is very slow at this time. If we would store the maximum id one could check if the id of a newly added object is greater than the stored one. If true the object cannot be in the list and we can safely add it. So it does not have to do with debugging but with optimization.


Trust me I know. What I mean is, we should only do that check (to see if a spatial is already added) in debug mode, and give a warning about it (because it shouldn't occur).
llama said:

Trust me I know. What I mean is, we should only do that check (to see if a spatial is already added) in debug mode, and give a warning about it (because it shouldn't occur).

Oh, well, I see.
I don't like stuff that behaves different when not in debug mode - do we really need a debug mode (except for more info of course)?

Let me rephrase myself, it should throw an exception even in debug mode. Perhaps another flag… jme.debug.checkduplicates.



But now that I think more about it, we really should consider dropping the check altogether, and start refusing (throw jME exception) to add Spatials that already have a parent by checking the parent field. (in my local copy I've already patched jME so that it now sets the parent field to null when a Spatial is removed). This could be done in any mode since it's just 1 check in all cases. Not removing a Spatial from a Node before adding it to another already messes up the structure, so it shouldn't be doable in the first place. With the coming changes, it'll be even worse to do that.



The question about doing different things in debug/release remains though. If there will me good speedups… why not? It should always work like assertions though… an error if the "different behaviour" occurs. In fact I'd use assertions to do it if it wasn't for 1.4.2 compatability.

Ok, using parent is by far more clever - so methods would look like this:


    public int attachChild(Spatial child) {
        if (child != null) {
            if (child.getParent() != this) {
                if ( child.getParent() != null )
                {
                    child.getParent().detachChild( child );
                }
                child.setParent(this);
                children.add(child);
                child.setForceCull(forceCull);
                child.setForceView(forceView);
                if (LoggingSystem.getLogger().isLoggable(Level.INFO)) {
                    LoggingSystem.getLogger().log(
                            Level.INFO,
                            "Child (" + child.getName() + ") attached to this"
                                    + " node (" + getName() + ")");
                }
            }
        }

        return children.size();
    }

    public int detachChild(Spatial child) {
        if ( child.getParent() == this )
        {
            int index = children.indexOf(child);
            if (index != -1) {
                child.setParent( null );
                children.remove(index);
                LoggingSystem.getLogger().log(Level.INFO, "Child removed.");
            }
            return index;
        }
        else
        {
            return -1;
        }
    }


While setParent should become protected. (other detatch methods changed accordingly)
(Should I check that in, already?)


Regarding the debug/release versions: I have made bad experience (in other projects) with assertions and other stuff that has the potential of changing bahaviour while debugging. Unexperienced developers (which we definately have in the community) tend to mess up with those (e.g. catching AssertionError), the resulting issues are quite hard to solve - if you look at it (turn debug mode on) the Problem vanishes...
IMO making a release version faster is fine but except for some less output and other information it should keep the behaviour that can be seen while debugging.

cough Shall I split this into another thread?

If you like  - though I could not specify at which point - the topic change was somehow continous :wink:

Ok, I'm going to respond to the original ID section (but we can discuss the other things too as they are good ideas).



First, I have completed the initial ID switch. Here's a bit of cut and paste from Spatial:



public abstract class Spatial implements Serializable {
   protected static final boolean useNames;
   static {
      useNames = Boolean.getBoolean("jme.debug.names");
   }
   
   private static int idCount = 0;
       //.... skipping stuff
        /** This spatial's id. */
       protected int id;
   
       protected static WeakHashMap nameMap;

/**
     * Constructor instantiates a new <code>Spatial</code> object setting the
     * rotation, translation and scale value to defaults.
     *
     * @param name
     *            the name of the scene element. This is required for
     *            identification and comparision purposes.
     */
    public Spatial(String name) {
        this.id = ++idCount;
       
        if(useNames) {
              if(nameMap == null) {
                 nameMap = new WeakHashMap();
              }
              
              nameMap.put(this, name);
        }
       
        renderStateList = new RenderState[RenderState.RS_MAX_STATE];
        localRotation = new Quaternion();
        worldRotation = new Quaternion();
        localTranslation = new Vector3f();
        worldTranslation = new Vector3f();
        localScale = new Vector3f(1.0f, 1.0f, 1.0f);
        worldScale = new Vector3f(1.0f, 1.0f, 1.0f);
    }

    /**
     * Returns the id of this spatial.
     *
     * @return This spatial's id.
     */
    public int getId() {
        return id;
    }
   
    public String getName() {
          if(useNames) {
             if(nameMap == null) {
                nameMap = new WeakHashMap();
             }
             return (String)nameMap.get(this);
          }
          return null;
    }
   
    public void setName(String name) {
          if(useNames) {
             if(nameMap == null) {
                nameMap = new WeakHashMap();
             }
             
             nameMap.put(this, name);
       }
    }



Another change is there were two getChild methods one took an int looking for a child and a given index, and the other took a string for the id. So, the previous int was renamed to getChildByIndex and the previous string was switched to int.

This caused the most changes through the source: lots of getChild(i) swtiched to getChildByIndex(i).

Two problems I still need to work out (any advice would be great):

1. In TerrainPage method fixNormals. Calls a significant number of private methods that look for a child based on its name, these are all commented out currently (Renanse you might be able to help me, because it's the fix you put in to smooth out the normals). Basically, I need another way to find surrounding terrain sections.

2. JmeBinaryWriter method writeTag. Haven't looked close yet, but this is throwing a nullpointerexception. I believe it is looking for model sections based on name.

Some things that I would recommend to change:


  • id should be final and private (has a public getter)
  • to avoid duplicate code call setName from ctor
  • it is unnecessary to create a new map in getName (simply add '&& nameMap != null' to the first if condition)
  • don't change getChild to getChildByIndex, all apps would have to be changed - add getChildById(int) instead
  • what about keeping getChild(String)? It could throw an exception if names are disabled
irrisor said:

- what about keeping getChild(String)? It could throw an exception if names are disabled


that would mean debug code works different from release code ;)

you’re right  :’(

Some things that I would recommend to change:

- id should be final and private (has a public getter)


done

- to avoid duplicate code call setName from ctor


done

- it is unnecessary to create a new map in getName (simply add '&& nameMap != null' to the first if condition)


done


- don't change getChild to getChildByIndex, all apps would have to be changed - add getChildById(int) instead


I feel that a getChild should retrieve the child identified by the parameter. I feel it will be more intuitive and remove confusion.

- what about keeping getChild(String)? It could throw an exception if names are disabled


I agree with llama here.
mojomonk said:

I feel that a getChild should retrieve the child identified by the parameter. I feel it will be more intuitive and remove confusion.

Hmm, existing code uses the getChild method at this time to iterate through the children of a node. Changing the baviour of getChild(int) would break all these code pieces (in code that is not available while refactoring). And even worse: the compiler will not notice it.

To behonest, I did not understand yet what the getChild(id) method will be used for :|.  Do people really expect it?
the compiler will not notice it.


That's a very good point. However, isn't it primarily internal code that does this now? All that has been converted. Purely conjecture, but I'd be willing to be most user apps don't manually transverse the tree.

Although, you've brought up a good point, so, if any other's (llama or renanse) agree, I'll switch it back.

If it's used much by others, don't break the API. If noone uses it anyway (I know I don't) go with what you feel looks best.