Node.sharedCopy() buggy?

Engines, Libraries and Tools Xith3D Forums
#1

I’ve spent a couple of days debugging and now I suspect that Node.sharedCopy() is buggy.

Here is the suspect code in Node.sharedCopy():


        } else if (node instanceof Shape3D) {

            Shape3D shape = (Shape3D)node;
            Shape3D newShape = new Shape3D();
            newShape.setAppearance(shape.getAppearance());
            newShape.setGeometry(shape.getGeometry());

            newShape.setBoundsAutoCompute(false);
            newShape.setBounds(shape.getBounds());
            newShape.updateBounds(false);
            newShape.setPickable(node.getPickable());
            newShape.setPickable(node.getRenderable());

            return newShape;

My problems go away when I use something much simpler like this instead:


       Shape3D  clone = new Shape3D (
         shape3D.getGeometry   ( ),
         shape3D.getAppearance ( ) );

  1. Is “newShape.setPickable(node.getRenderable());” supposed to be “newShape.setRenderable(node.getRenderable());”?

  2. Why does Node.sharedCopy() turn off bounds updating?

#2

  1. looks like a bug

  2. I’m not sure

Does it make a difference which way it is initialised? I havn’t had problems with it in the past.

Will.

#3

Let’s agree to fix the first bug just because it is obviously an error. Is the original author available to confirm?

Let’s discuss fixing the second bug. The code duplicates the node bounds in the shared copy. This means that the shared copy might not be rendered unless it is in the same position as the original. When would you ever create one or more shared copies with the same position as the original?

This bug is truly mystifying as it makes the shared copy invisible since the geometry and the bounds do not overlap. This bug cost me two days of debugging because it did not work as I expected it to. Most of that time I thought the problem was in my code. Let’s fix this bug by deleting or remarking out these lines:


            newShape.setBoundsAutoCompute(false);
            newShape.setBounds(shape.getBounds());
            newShape.updateBounds(false);
#4

One way we could bypass the issue as to whether to fix sharedCopy() or not is to implement cloneNode() which is currently missing from the Xith implementation. We could deprecate sharedCopy() but keep it there for those currently using it.

Here is the javadoc for cloneNode():
http://java.sun.com/products/java-media/3D/forDevelopers/J3D_1_3_API/j3dapi/javax/media/j3d/Shape3D.html#cloneNode(boolean)

#5

I hate the idea of leaving bad code around for someone else to step in. I say fix it. I would not be to concerned with ownership of the original developer. You have made a could case for fixing this code.

#6

Yeah fix it - and maybe add a note in the javadoc. That they should, if the old code should keep working, do that and that.
OR write another function (e.g. cloneNode) and mention it in the javadoc, then nobody can step in that easily anymore and others can still use the old code.

#7

But:

Does it make a difference which way it is initialised anyway? I havn’t had problems with it in the past.

I would like to confirm the existance of the problem before worrying about the work around. As far as I can see there is one obvious bug, a second possible bug - the order of initialisation shouldn’t matter (though it might), and I’m not quite sure why sharedCopy is disabling boundsUpdating and what the proposal is to do instead?

Will.

#8

Based on my experience of two days debugging, it does. The initialization does not act as one would expect it to. Here is the javadoc for the code in question:


    /**
     * Creates a shared copy of the given Node.
     * A shared copy is one where the geometry and appearance is shared, but all other
     * nodes are copied.  This is a replacement for shared groups because of
     * performance considerations.  If you are loading the same model many times
     * then this can save on memory and load times.  The only allowable within the subtree
     * are groups and shapes. This also copies a shapes bounds and turns autocomute off
     * so that it is fast to insert the model into the scene.
     * 
     * @param node The Node to be copied
     * @return a shared copy of the given Node
     */

Based on the above, it looks like bounds are copied and fixed for speed of insertion. I’m not sure this is justified given the confusion it causes. Is the user of this method supposed to know that they must then reenable bounds if they want their shared copies to have non-identical locations?

The two authors of Node.java are Scott Shaver and David Yazel. I understand that David Yazel is no longer around but how about Scott Shaver?

#9

Thanks for the info. David was definitally the author of that code, I don’t think Scott is the best to ask, so it’s just us :slight_smile:

I think I agree that the confusion over bounds isn’t worth the speed improvement, at the very least we can override the method and give the user the option to turn it off (if by defualt we turned it on).

Do you think many people would be affected should we change the initialisation, fix the other bug and turn on bounds calculation? It seems to me this is a viable option.

Will.

#10

How many do you suppose are currently using sharedCopy()? I suspect it cannot be many or these bugs would have been flushed out earlier. Of those that are using it, I would guess that they must be re-enabling bounds manually after initialization.

#11

I thought about it overnight. Let’s leave sharedCopy() unchanged but deprecate it and implement the missing cloneNode() method.

#12

Where are we on this? Are we ready to proceed with the bugfixes?

#13

+1 for cloneNode

#14

+1 for cloneNode

#15

Yes, lets do it. I am insanely busy this week, but have some time on the weekend to make/merge any changes.

Will.

#16

OK, what do you think of this as a first draft in an incremental approach?


    public Node cloneNode(boolean forceDuplicate)
    {
      try
      {
        Node  node = ( Node ) getClass ( ).newInstance ( );
      
        node.duplicateNode(this,forceDuplicate);
      
        return node;
      }
      catch ( Exception  ex )
      {
        RuntimeException  runtimeException = new RuntimeException ( );
        
        runtimeException.initCause ( ex );
        
        throw runtimeException;
      }
    }

This is based on the Sun javadoc for cloneNode().

#17

What does

node.duplicateNode(this,forceDuplicate);

do?

Do we have to implement it, or does it work correctly by default? Have you tested it?

Arne

#18

We would have to implement it. It would be a next step in this incremental approach.

The Xith implementation of class Node currently has a duplicateNode() method as an empty placeholder. After we write cloneNode(), we would need to tackle duplicateNode() and cloneTree() as well.

#19

Fair enough, looks good.

Will.

#20

OK, what is the next step? Do we update the code now?