[Odejava] OO Issues

Game Mechanics
#1

[Cross post from dev@odejava.dev.java.net]

Jani & Odejava Users,

Do you think there are any ways to improve the current Ode Objects? The
new high level Odejava objects are a very good step in the right
direction however there are still some issues that I feel are holding
them back. The issue I have is that when you create say a Joint object,
the object is added to the Space automatically. I understand this is
due to the C roots of ODE but I feel it would be nice to have an even
more Object Oriented approach.

Here’s an example (taken from the CarExample code):

        Geom g;
        g = new GeomBox("ramp", 80, 32, 2);
        g.setPosition(new Vector3f(0, 0, 20));

What this code does is create a new ODE Geom object AND add it to the
current ODE Space. While it is a far cry from using the low level C
calls, it’s still not all that intuitive.

Conversely, with say Xith3D or Java3d, you construct your objects and
then add them to a locale/group/whatever manually:

  BranchGroup bg = new BranchGroup ();
  // ...
  locale.addBranchGraph(bg);

In other words, Xith3D objects are added explicitly whereas OdeJava ones
are added implicitly.

The Xith3D model has several advantages, not the least being readability
and also that you can add a BranchGroup to one scene - then say ten
minutes later it can be added again to another scene (assuming it was
removed from the first one). With OdeJava one would have to call the
constructor of all those methods again. Like with BranchGroups, Odejava
Geom’s would have to be acyclic.

Having Odejava and Xith3D share the same model would also help
readability immensely.

I believe this change is more of a design decision than a large coding
effort. As far as I can tell, it wouldn’t be hard to refactor the
current objects.

  • Geom.java would need a new abstract method “addToSpace(Space space)”

  • GeomBox.java for example would need this change:

    Local variables: float sizeX, float sizeY, and float sizeZ in the

constructor would need to be instance variables

  • GeomBox.java would need to implement the addToSpace method and have it
    contain the Ode.dCreateBox ODE constructor.

So the main changes will be a new method in the Geom’s so they can be
added to a Space, and storing the initialisation data in the geom.

What are your thoughts please?

Cheers,

Will.

#2

Doesn’t Space already have a method addGeom(Geom)? The constructor of the geoms could be changed to not call this method automatically. (I currently don’t have the time to look at this in detail, so sorry if I am off the beam.) And yes, I agree with your thoughts. :slight_smile:

#3

Jens,

Admittedly I havn’t noticed that method but the problem is a little deeper than that.

Here’s the constructor for GeomBox:


      public GeomBox(
            String name,
            Space space,
            float sizeX,
            float sizeY,
            float sizeZ) {
            super(name, space);
            geomId = Ode.dCreateBox(spaceId, sizeX, sizeY, sizeZ);
            updateReferences();
      }

Ode.dCreateBox is the line which is adding the box to ODE. Also - all of those variables are actually temp variables, so currently this operation can only be performed in the constructor (yes it’s a trivial change I know).

Perhaps one way of doing it is to add an abstract method to “Geom” which is like “addToSpace(Space space)”;

Essentially what I am saying is that it would be nice for these objects to have a use after construction. Currently that’s all they are good for. It’d be nice to be able to add them to spaces, remove them from spaces, change their size etc… We can also code in Serialization as a nice by-product of this.

Xith3D/Java3D serve as a good model for this IMHO. A physics engine is quite similar to a 3d scenegraph in many ways just with the results being mathematical not visual. In fact due to the fact both have similar results (some kind of virutual world) but different ways of getting thereAPI’s, it makes it harder to think it out.

Will.

#4

Higher Level (HL) API is simple as the underlying ODE does not require for the HL API to be complex. So putting this API in shape should not be a big task. Current HL API was ‘designed’ and implemented in a weekend or so, actually it was more of getting Odejava 0.2 released than properly designing API and thus postponing release again with weeks…

Currently all Geoms that are created are implicitly added to Odejava.getCurrentSpace(). So if you want to add Geoms into different spaces then you have to change your ‘current space’ by calling Odejava.setCurrentSpace() each time you’re hopping to another space. This is of course completely wrong design. Note that Odejava class has also currentJointGrouop and currentWorld, these are quite analoguous to currentSpace.

At least Geom.java and Body.java should be changed.

One option is to add Space into Geom’s constructor (this affects all non abstract Geom classes, like GeomBox, aswell).

Another option would be when Geoms are created, add them always to non-existent space (zero). Then create addToSpace method and use this to set Geom into a space that developer wishes. I think this is good solution, and is exactly what Will suggested.

Same thing should be done for currentJointGroup. Initially they go to zero (“non-existent limbo”), and developer needs explicitly to call myJoint.addToJointGroup(myJointGroup)

I’ve been waiting for this conversation to happen :slight_smile:

#5

[quote][Cross post from dev@odejava.dev.java.net]

  • Geom.java would need a new abstract method “addToSpace(Space space)”

  • GeomBox.java would need to implement the addToSpace method and have it
    contain the Ode.dCreateBox ODE constructor.
    [/quote]
    I’d like to implement mySpace.addGeom(myGeom) instead of myGeom.addToSpace(mySpace). I assume this is what you meant.

Yes, this might be good also. There’s other classes also where data would be nice to be accessed through instance variables.

Instance variables are nice but they might have some issues as ODE gives the final word what is their value. Also they are redundant, but on the other hand save us from JNI calls. The main issue I could think of is that if something (lower level API calls, ODE itself) changes these variables then Java instance variables are out of synch. Hence the simplistic “non-java style” implementation of float[] GeomBox.getLengths() that returns x,y,z lengths in a single call. Perhaps adding more conviniency methods would do the trick, like getLengthX() etc?
Other option is to trust that ODE does not change these values without Odejava’s knowledge, this is the case with GeomBox classes x,y,z sizes.

Summarum:

Initially all created Geoms or Joints are in limbo, they do not affect simulation in any way.

One short gallup for two prospect methods:

a) mySpace.addGeom(myGeom);
vs.
b) myGeom.addToSpace(mySpace);

a) myJointGroup.addJoint(myJoint);
vs.
b) myJoint.addToJointGroup(myJointGroup);

I go for version A if this is ok for everyone? This resembles ODE’s C++ headers and scenegraph libraries like Xith3d.

GeomBox’s sizes (x,y,z) are put on instance variables, proper getters (no JNI used) and setters (JNI used) are added, but constructor still requires sizex,y,z parameters. Same follows for other geoms like GeomSphere. Ok?

#6

Definitally A is better, I agree - but you could always have both (myJointGroup could simply call myJoing.addToJointGroup(this)).

[quote] One option is to add Space into Geom’s constructor (this affects all non abstract Geom classes, like GeomBox, aswell).

Another option would be when Geoms are created, add them always to non-existent space (zero). Then create addToSpace method and use this to set Geom into a space that developer wishes. I think this is good solution, and is exactly what Will suggested.
[/quote]
I don’t like the first option. The second option is ideal, apart from anything else it will make Odejava code much more readable.

Regarding the instance variables - what I would like to see is definitally the Ode objects being “useful” after creation be it though instance variables or JNI calls, callbacks. A few more methods like “mySpace.remove(Geom)” and ones like that would be nice too.

Will.

#7

would you mind if I went ahead and did these changes? I’d be happy to refactor the demos as well.

Will.

#8

[quote]would you mind if I went ahead and did these changes? I’d be happy to refactor the demos as well.

Will.
[/quote]
Yes! I’ve been anticipating this :slight_smile:

#9

Just to make sure I am on the right track - if I want to use this nothing space - I just use this as my spaceId:


 new SWIGTYPE_p_dSpaceID(0, true);

yes?

Will.

#10

Yes, you could use also Ode.getPARENTSPACEID_ZERO() method, it has the same effect. Actually better name for this method should be simply: Ode.getSPACEID_ZERO(), just like the other get*ID_ZERO() methods.

#11

Ok, I’ll use that. Infact I had just created a SPACEID_ZERO static object as part of the Space class - maybe I’ll still keep it just for conveniance (but have it call getSPACEID_ZERO() instead).

The conversion is progressing - I would have finished it yesterday had I not fallen asleep :-/

In advance I apologise for anyones code I will break…

Will.

#12

I’ve been playing around trying to get the optimal mix of readability vs. succinctness (with emphesis on the former) into the high level objects. I have drawn upon Swing, AWT and Xith3D/Java3D as references.

Basically I am elimitation anything that’s not explicit. In other words - Odejava.getCurrentX() and removing Space from Body (infact there is a todo in the source to do just that). The main problem with these static calls is that unless you have the source in front of you, you think “how does it know which space to add my body too?”. Removing everything todo with Space from the Body class is also a good think I think - the programmer then must do this explicitly.

BodyS with Geom will be created like this:


Body b = new Body("example", world);
Geom g = new GeomBox("blar", 1, 1, 1, 1);
space.addGeom(g);
b.setGeom(g);

Instead of


Body b = new Body (new GeomBox("blar", 1, 1, 1, 1));

which actually does the same thing (with the old version);

Anyone who can’t live without the conveniance of the old version can always create some helper classes… but it really boils down to selecting a few extra lines in your copy/past.

As you can see, the proposed changes remove all “hidden” and non-explicit stuff and make everything easy to see at the surface level.

I think that totally getting rid of Odejava.getCurrentX() methods is wise - the programmer is free to code their own equivilent (taking all of 3 seconds).

Please tell me what you think :slight_smile:

I’ve got some more refactoring and testing to do before it’s ready.

Will.

#13

Will, just go for it.

Explicit process (even if longer) is definately better than wrapping up things implicitly under some class. The old implementation was done in a flash, it’s bad just as you and source comments stated (“these should be removed”).

#14

hehe, that’s fair enough - best to clean it up sooner rather than later :slight_smile:

Sorry for the delays, I keep changing what I am doing (partly because I am still learning ODE).

The current line of thought is that while I will remove all references to Space from Body - I’ll keep the references in Space to Body, as the BodyS do often have collision info attached.

I think the best way will be just to have an “add” method of space - which either takes a plain Geom, or a Body (which has a Geom).

Will.

#15

the org.odejava.joint package has now been cleaned up, removing all references to getCurrentWorld and getCurrentJointGroup.

The side effect of this is that you know must pass a World and JointGroup object when constructing them. All code which uses joints will need to be updated in this fashion.

Will.