Add detach() method to Group and deprecates BranchGroup ?

Engines, Libraries and Tools Xith3D Forums
#1

What do you think ? Here’s the code in BranchGroup ?


public class BranchGroup extends Group {
    /**
     * Constructs a new BranchGroup object.
     */
    public BranchGroup() {
        super();
    }
    
    /**
     * Detaches the BranchGroup node from its parent. Sets
     * the node to not live.
     */
    public final void detach() {
        
        if (View.CHECK_FOR_ILLEGAL_MODIFICATION) View.checkForIllegalModification(this);
        
        Node p = getParent();
        if(p!=null)
        {
            if(p instanceof Group)
                ((Group)p).removeChild(this);
            else
            {
                throw new SceneGraphRuntimeException("BranchGroup.detach(): An unknown node type is the parent.");
            }
        }
    }
}

I think it would simplify the code a bit.

#2

And also methods in the Xith3D API (even in the tk) should take/return Groups in argument, not BranchGroups.

#3

I’m absolutely for it. It is a little confusing, if a BranchGroup is used anywhere inside the scene branch.

But for Java3D compatiblity we should leave the BranchGroup non-deprecated and use it as a brach-root-group. And I’ll use BrachGroup for the new multipass code.

So as far as I am concerned, please refactor the detach() method to Group and shrink BrachGroup usage to a minimum (all points except Locale.addBrachGraph()).

Marvin

#4

Don’t really the point of the Java3D compatibility thingy but OK.

#5

[quote="<MagicSpark.org [ BlueSky ]>,post:4,topic:28344"]
Don’t really the point of the Java3D compatibility thingy but OK.
[/quote]
Well, BranchGroup is the group type that you use with the Locale.addBrachGraph() method in Java3D. So strictly speaking it would break the Java3D “compatiplity” if we would deprecate it. But I think it’s no thing, anyone wouldn’t instantly understand. The method takes a specific group type and you’ll know what to do. So no problem till this point.

But I find it logic to have a different group type as the root group --> BranchGroup. And it would give us the possibility to be more flexible and do things with the root group than with all the other group… just as I am planning with multipass ;).

#6

Well, BranchGroup is the group type that you use with the Locale.addBrachGraph() method in Java3D. So strictly speaking it would break the Java3D “compatiplity” if we would deprecate it. But I think it’s no thing, anyone would instantly understand. The method takes a specific group type and you’ll know what to do. So no problem till this point.

But I find it logic to have a different group type as the root group --> BranchGroup. And it would give us the possibility to be more flexible and do things with the root group than with all the other group… just as I am planning with multipass ;).
[/quote]
OK-dokey.

#7

Done. Had a great manual merging party today… thanks Qudus ;D ;D No just kidding. Putting detach() in node is even better.

But why did you delete the getRenderFrame() method in View ?

#8

Hah, I saw you added a Renderer class. Gonna dig that a bit.

#9

[quote="<MagicSpark.org [ BlueSky ]>,post:7,topic:28344"]
Done. Had a great manual merging party today… thanks Qudus ;D ;D No just kidding.
[/quote]
Sorry ;D I thought I was the only one working at this late time (for us Europeans).

[quote="<MagicSpark.org [ BlueSky ]>,post:7,topic:28344"]
But why did you delete the getRenderFrame() method in View ?

Hah, I saw you added a Renderer class. Gonna dig that a bit.
[/quote]
I didn’t add this class. It was there before my recent changes. I just moved the rendering code from the View to the Renderer class and the Atom collecting code in the FrameAtomsCollector class which previously was named RenderFrame.

I always wondered what this RenderFrame class did when it didn’t actually render. I browsed through the code and found out, that it actually sums up all the Atoms. So I renamed it to FrameAtomsCollector to make it clear to everyone what it actually does and moved the code from Renderer to it, which actually belongs to it. On the other side I moved the rendering code, which was residing in the View class into the Renderer class, since the View only describes a camera or an Eye and doesn’t actually render anything.

I think this is much better understandable, isn’t it. So we all can better comprehend how the rendering works.

The Renderer actually doen’t render itself. It just flatens the scenegraph to Atoms which the actual renderer (OpenGL) can handle better.

I’ll proceed this work now and maybe rename some more classes / methods to improve understandability and logical correctness of the code. After all I’ll have a quite good understanding of the render process :slight_smile:

Marvin

#10

OK. But be sure to notify us on the forums on our changes.

Seeing a crucial method like getRenderFrame() disappear on CVS without an explanation is… surprising.

#11

[quote="<MagicSpark.org [ BlueSky ]>,post:10,topic:28344"]
OK. But be sure to notify us on the forums on our changes.

Seeing a crucial method like getRenderFrame() disappear on CVS without an explanation is… surprising.
[/quote]
You know I’ve (most) always reported my changes in the forum. And I would never do such a big change without notifying. But I wanted to wait until I’ve finished this work. And so far it is not. I think, I’ll have finished it this night.

#12

For the Background / Foreground .getGeometry() method:

I was always disturbed of this method, since it doesn’t return a Geometry or Shape and always wanted to rename it. But It has this name in Java3D. So I din’t do it so far. I just stumbled over it again last night :slight_smile:

Anyway, I really like it renamed, since it is a failure we don’t need to imitate from Java3D and it’s one more point we are better in than Java3D, since it is better understandable. So it’s good, that you renamed it, Amos.

#13

Well it just disturbed me that a method named getGeometry() returned a Group. ;D (probably it was a geometry in the ancient times).