Appearance.set<Atribute> - probably buggy???

Hi!

I am not sure if this is already known, or if this is the bug at all, but it seems to me so, so I decide to report.
Here is description and test case as well, so you can check it fast. Would appreciate if somebody can confirm it.

The problem is, that if you have Shape3D with the Appearance applied (let’s say “myApp”), and if this Shape3D is currently invisible, just behind your field of view for instance, then changing any of the attributes of “myApp” not neccessary will have efect, well, most of the time it will, but eventualy - not.
The test case is simply bunch of quads, spread over square area with the camera located roughly at its center. The square is constantly rotating so that you see roughly half of it at a time. While in rotation the Appearance (same applied to all the quads!!!) is modified (coloring attr. for example) on regular intervals. Now have a look what you actually going to get from that…
Is it proper behaviour? BTW, I found experimentaly how to actually avoid the problem (see and try the comment at the end of code)…

It was tested with the latest Xith _0.7.1 on JSR-231, Windows, and few different PC’s (different video-cards).


import java.awt.Color;

import javax.swing.JFrame;
import javax.vecmath.Color3f;
import javax.vecmath.Point3f;
import javax.vecmath.Vector3f;

import com.xith3d.render.CanvasPeer;
import com.xith3d.render.RenderPeer;
import com.xith3d.render.jsr231.RenderPeerImpl;
import com.xith3d.scenegraph.Appearance;
import com.xith3d.scenegraph.BranchGroup;
import com.xith3d.scenegraph.Canvas3D;
import com.xith3d.scenegraph.ColoringAttributes;
import com.xith3d.scenegraph.Geometry;
import com.xith3d.scenegraph.Locale;
import com.xith3d.scenegraph.Shape3D;
import com.xith3d.scenegraph.Transform3D;
import com.xith3d.scenegraph.TransformGroup;
import com.xith3d.scenegraph.View;
import com.xith3d.scenegraph.VirtualUniverse;
import com.xith3d.test.TestUtils;

public class AppearanceChangeTest {
    
    public AppearanceChangeTest() {
        JFrame frame = new JFrame("AppearanceChangeTest");
        frame.setSize(500, 500);
        //======= setting up Universe ===============
        VirtualUniverse My_Universe = new VirtualUniverse();
        Locale locale = new Locale();
        My_Universe.addLocale(locale);
        BranchGroup scRootBG  = new BranchGroup();
        locale.addBranchGraph(scRootBG);
        //------------------------------
        RenderPeer renderPeer = new RenderPeerImpl();
        CanvasPeer canvasPeer = renderPeer.
                       makeCanvas(frame.getContentPane(), 0, 0, 32, false);
        Canvas3D scrCanvas = new Canvas3D();
        scrCanvas.set3DPeer(canvasPeer);
        //------------------------------
        View scView = new View();
        scView.addCanvas3D(scrCanvas);
        My_Universe.addView(scView);
        //------ rotation group --------
        TransformGroup rotTG = new TransformGroup();
        scRootBG.addChild(rotTG);
        //======== adding many shapes3d ===============
        Appearance app = new Appearance();
        ColoringAttributes colAtt1 = new ColoringAttributes(
                new Color3f(Color.RED), 0);
        ColoringAttributes colAtt2 = new ColoringAttributes(
                new Color3f(Color.BLUE), 0);
        app.setColoringAttributes(colAtt1);
        float dL = 0.1f;
        for (float i = -5; i < 5; i++) for (float j = -5; j < 5; j++) {
            Point3f P1 = new Point3f(     i+dL,     j+dL, 0);
            Point3f P2 = new Point3f(     i+dL, j + 1-dL, 0);
            Point3f P3 = new Point3f( i + 1-dL, j + 1-dL, 0);
            Point3f P4 = new Point3f( i + 1-dL,     j+dL, 0);
            Geometry geom = TestUtils.createQuad(P1, P2, P3, P4, 1, 1);
            rotTG.addChild(new Shape3D(geom, app));
        } 
        //======= running =============================
        frame.setVisible(true);
        scView.getTransform().lookAt( new Point3f (0, 2, 2),
                                      new Point3f (0, 0, 0),  
                                      new Vector3f(0, 0, 1));
        float a = 0;
        long currTime = System.currentTimeMillis();
        long lastTime = currTime;
        int count = 0; // 
        Transform3D rotTF = new Transform3D();
        while (frame.isVisible()) {
            scView.renderOnce();
            //-----------------------------------
            currTime = System.currentTimeMillis();
            if (currTime - lastTime > 20) {
                lastTime = currTime;
                a += 0.03f;  if (a > 2*Math.PI) a -= 2*(float)Math.PI;  
                rotTF.rotZ(a);
                rotTG.setTransform(rotTF);
                count++;
                if (count > 50) {
                    count = 0;
                    if (app.getColoringAttributes() == colAtt1) {
                        app.setColoringAttributes(colAtt2);
                    } else {
                        app.setColoringAttributes(colAtt1);
                    }
                    //--- uncomment both lines to fix the problem ---
                    //rotTG.setLive(false);
                    //rotTG.setLive(true);
                }
            }
            //------------------------------------
        }
        System.exit(0);
    }   
    
    public static void main(String args[]) {
        new AppearanceChangeTest();
    }
}

Bohdan.

Had anybody chance to look at it?
Do I do something wrong or it is a bug?

Bohdan

I ran it and saw the things exactly how you described it.

BTW, you don’t need the second setLive()

I looked at the code of SceneGraphObject and Appearance but there’s nothing justifying that behavior… Others devs, any idea ?

Thanks for reply!

I will dig into the code too and try to spot smth…

[quote="<MagicSpark.org [ BlueSky ]>,post:3,topic:27732"]
BTW, you don’t need the second setLive()
[/quote]
aha… :slight_smile: alright…

Ok… I have found bug which causes this problem.

So, the story is the following.
We have banch of Shapes which share same Appearance - ok.

Strarting rendering the frame:

  1. View.getRenderFrame(Canvas3D canvas)… => … => View.renderNode(…)
  2. in “View.renderNode(…)” : if (!node.isIgnoreBounds()) { … cull the bastard :)))) } - frustrum culling - this is important point, so shapes which are not visible are nod going to be render… this is very good… but!!! next:
    //----------------------------------------------------------
  3. if shape is not culled => renderer.addAtom(getAtom(shape));
  4. in getAtom(shape) : if (atom == null || aChanged) { atom = new ShapeAtom(shape,viewStack.getTop()); here we refreshed appearance for that shape;
  5. still in “getAtom(shape)”: if(aChanged) changedAppearances.add(a);!!! - again important to note!!!
    here note, that all our shapes share same appearance, so basically changedAppearances list will be populated with many references of same Appearance after rendering is complete;
  6. now back to “View.getRenderFrame(Canvas3D canvas)”, after rendering finished: - again important for this bug:

for (int i=0;i<numChangedA;i++)
((Appearance)changedAppearances.get(i)).setChanged(false); <<-- here we actually many times set to the same shared appearance changed “false”… and off cause all those shapes which actually where not rendered because they where culled - they will never get modifiyed appearance applied bacause Appearance.changed is already set to “false” :)))

Bohdan.

P.S. Well, I think this is difinitely that, and I think it is pretty easy to correct, ok, I make fast correction for myself, and everything is fine.
But this is not effecient correction, I think there are few things to correct. First off all, the appearance status (changed/not changed) can hold the shape itself somehow not the Appearance… Secondly, it is fairly common to share appearance so situation discussed when you iterating through the 100s same refferences - is not the best solution too and I think it is easy to correct too.

If you wish - I can correct that all (well, I paltry did), well I will try to found more efficient solution and will post it so somebody from devs can commit it.
Is that ok?

So it seems like you will have to use one Appearance for each Shape3D. Simply Write a method which creates it, so you won’t have code duplication. Is this a performace problem for you?

Ok… but is that the solution - to have individual Appearances? The bug still exist and somebody will be caught on it like me…
And onother thing you have 1000 shapes with the same appearance, ok… you want to change the color… are you going to iterate through 1000 appearances?

Well this is an issue which takes time each time you change the color (or something else) but if we took away this isChanges flag, we’d loose perfromace each rendering iteration. I think it is better to loose it once, isn’t it? But maybe there is just another, even better solution.

No-No!! Of cause I’m not talkin about removing the flag… I suggest, we can move that flag from Appearance to the Shape!!! no performance loss at all!!!

hmm… good point. Who is familiar with the rendering code and could check/do this?

And actually… this flag isChanged, accordingly to the rendering scheme and definitions of Shape etc… (ok, as for me personaly) - is actually more “applicable” really to Shape, not to the Appearance itself, this is more logically I think… What you say?

Look, it is even looking nicer: rather than having flag in appearance which indicates that it was changed (even though, having it set you not going to do anything with that appearance, you have changes committed straigth away…) - it will sit in Shape and indicates that its appearance was changed and needs to be “reapplied” - sounds better!!!

And, more to it, having that done - we don’t need that “changedAppearances” list at all!!! because as soon as we “reapplied” appearance for shape we reseting its flag (shape’s flag) at the same time - nothing needs to be stored for latter “clearance”!! So actually - it even increases performance, well at least little bit + code would be more consistent and clear.

Definitely go for it. I’ll commit it.

Ok, thanks :slight_smile:

So I’ll post the changes I suggest shortly (today hopefully if I have a time).

Bohdan.

Ok, here are the changes:

Since “changed” flag is actually the flag of super class (NodeComponent), I didn’t really “move” it (to make sure I will not brake anything), but the code below will act same way really.

Appearance will have now its unique indentifier “changeID” that is updated each time (see getAtom(…)) any of the Appearance attributes change. Change identifier for Appearance is updated only once per frame rendering (if appearance was changed) even if appearance is shared by many Shapes3D.

At the same time each Shape3D has its own “appearance change identifier” which is to be compared to appearance’s one. If it is not equal - appearance would be reapplied, otherwise - not. If change was applied corresponding identifiers become equal. This allows considering Appearance changes on per Shape3D basis rather than on per Appearance basis as in current implementation, and still having Appearance sharing in place.

Please have a look at the code and comment if you feel something is wrong. There should be no compatibility problems, since no methods which may be potentially used outside of frame rendering procedure were modified. The setChanged(…) method is unafected - you still can used it (if you ever need it really…) and it will work in this scheme with no problems too. I don’t see any performance loss here, frequent/excessive memory allocations or extra garbage collection… (and changeID is never set to null, except the only case - when Appearance is set to null).

So, the actually code:

In com.xith3d.scenegraph.Appearance (nothing to modify, just add the following):


+ private Object changeID = null;
+    
+ protected final Object verifyChange()  {
+       if (isChanged())  {
+             changeID = new Object();
+             setChanged(false);
+       }
+       return changeID;
+ }

In com.xith3d.scenegraph.Shape3D (nothing to modify, just add the following):


+ private Object appChangeID = null;
+   
+ protected boolean verifyAppChange()  {
+     if (appearance != null)  {
+           Object change_id = appearance.verifyChange();
+           if (change_id != appChangeID)  {
+                 appChangeID = change_id;
+                 return true;
+           }
+     } else if (appChangeID != null) {
+            appChangeID = null;
+            return true;
+     }
+     return false;
+ }

In com.xith3d.scenegraph.View.getAtom(Shape3D shape) :


- Appearance a = shape.getAppearance();
- boolean aChanged = (a != null && a.isChanged());
- if (atom == null || aChanged) {
+ if (atom == null || shape.verifyAppChange()) {
- if(aChanged) changedAppearances.add(a);

In com.xith3d.scenegraph.View.getRenderFrame(Canvas3D canvas) (remove, as not needed anymore)


- final int numChangedA = changedAppearances.size();
- for (int i=0;i<numChangedA;i++)
-     ((Appearance)changedAppearances.get(i)).setChanged(false);
- changedAppearances.clear();

In com.xith3d.scenegraph.View: (remove declaration as unused)


- ArrayList changedAppearances = new ArrayList();

Bohdan.

Committed.

Thanks!!!