The way we use the classes CollisionDetection, Intersection, and Pick don’t seem very OO to me. If we use Sun’s JDK as any example, I don’t see a single place they use classes the same way.
For example, instead of (translate the C++ shorthand to java )
public static boolean Intersection:: Intersection(Bounds,Bounds);
what about
public boolean Bounds::testIntersection(Bounds);
The above is being implimented by mojo?
and instead of
public static void CollisionDetection(Spatial,Spatial,CollisionResults)
what about
public void Spatial::storeCollisions(Spatial,CollisionResults)
and instead of
public static void Pick::doPick(Spatial,Ray,PickResults)
what about
public void Spatial::doPick(ray,results)
I propose we add storeCollisions and doPick to spatial as an empty function. Overload the function in Node to carry it out on the children. Overload the function in TriMesh to actually do the collision detection and picking.
Also, in a manner similar to AWT, instead of forcing people to use and access our CollisionResults class and our PickResults class I suggest we define the interface CollisionResults and the interface PickResults so that users impliment them (or define anonomous inner classes) to do what they want, if using the information once you get it happens to be more efficient or OO to your design than storing and accessing it later.
I can do the above for review if it passes the "discuss in the forum" stage of our review.
personally, i quite like the current collision detection system whether its very OO or not. Because its very easy to setup and gives back which geometry you have intersected with.
I think the current system is one of jME’s advantages at the moment because you wouldn’t need to worry about Trimeshes and bouding volumes…etc
It also integrates quite nicely in the current AISystem because it centrelises the collision detection in a single “block”.
The system i describe is very easy to setup and is much more customizable. Intersections of BoundingVolumes and intersections of Triangles are two different task so should have their own routines. Usually, just one of the two will do and the other is unneeded computation. Collision detection will work just like it does now for most things, but instead of refrencing an outside, 3rd party class to do the work, you refrence the actual object that is actually doing the work. Code is also more human readable.
Does he just want to know if two spatial’s bounding volumes intersect, or is he wanting to know which spatials of the two sets of spatials (IE the two nodes intersect).
very descriptive. I can tell the two bounds are being tested for intersection
Spatial a,b;
// recurses through a and b's children if they are nodes.
a.findBVIntersection(b,new IntersectionTest(){
public void onIntersection(Spatial a,Spatial b){
System.out.println("The spatial " + a.getName() + " intersects with spatial " + b.getName());
}
}
Very easy to tell what's going on with that code
Spatial a,b;
// every leaf of A against every leaf of B
a.findTriIntersections(b,new CollisionDetection(){
public void onCollision(TriMesh a,int trianglea,TriMesh b,int triangleb){
//... color A's ath triangle blue and B's bth triangle red
}
That's the system I imagine for the best blend of speed and customizability.
I don’t have a problem with a revamp of these systems. In fact, I think we need to figure out a way to clean up the BoundingVolumes as well. Remove the TriMesh dependancy.
I wouldn’t mind taking the BoundingMesh / Trimesh dependencies on as my next task… if that was decoupled enough to where I wouldn’t be getting in Ceps way (it should be, the basic stuff will still be the same)
If you do this I think that you should make node override findBVIntersection so the test goes down the list. (It returns the Spatial ). And do the same with he other methods.
Node::findBVIntersection(Spatial b,IntersectionTest c)
{
If(this.getWorldBound().isIntersecting(b.getWorldBound))
onIntersection(this,b);
for( all Spatial in me)
(child i).findBVIntersection(b,c);
}
I also think that three should be a method in jme for getting a list of all the collisions in the seane. (Other then just looping threw all the nodes, something like a tree.)
What should an OBBTree depend upon? It seems like a “contains” OOP relationship. The “TriMesh” object “contains” an “OBBTree”, which would make it a member variable of TriMesh objects in OOP terms. Being a member variable of TriMesh objects, it should be the TriMesh class that operates on it.
In my local CVS, I made the changes (just to see how it works), and here’s how easy it is to use for me.
I load an obj model.
I can updateBoundingTree() on the root node of the obj model, which propgates calls down to leaf nodes
During simpleUpdate I call testIntersection on the obj model’s root against a “bullets” node that contains various bullet objects. I get a list, and change colors. It flows pretty smooth.
How do you suggest we impliment the system?
Edit: Badmi, I agree. Nodes should propgate calls down to children.
BoundingVolume extending Trimesh will be removed. That’s what I mean by the dependancy.
I’m not quite sure why there are three seperate threads about this. It’s being worked on right now. Just hold off for a bit until I’ve finished what I’m working on, then these conversations will have more relevance.
If we are at subject of BoundingVolumes, I would suggest changing
public void initCheckPlanes();
public int getCheckPlane(int index);
public void setCheckPlane(int index, int value);
InitCheckPlanes does not belong to interface in any case (it is never called by outside code and is part of object initialization). As for the other two methods, I think they are overcomplicated. As far as I understand, idea is to cache last frustrum plane which object was rejected by. For this, only one int is enough - why cache other 5 ints with it ? In theory, it could cache other solutions in order of probability, but it is not done by current code (it just replaces swaps top one with reject plane, without arranging rest in any order) and probably is not worth the trouble - in case of serious camera movement, there is a big chance that order will be wrong anyway.
I would suggest simplifying it to something like
public int getCheckPlane();
public void setCheckPlane(int index);
(maybe also rename get/setCheckPlane to get/setLastRejectionPlane ? or just get/setFrustrumHint() ?)
Ok… The code for checking the planes was written and rewritten and has resulted in good performance. I don’t see any real reason to change it because you feel it is too complicated. You somehow have to cycle through each of the planes in the camera checks regardless of which one is checked first, so if you don’t keep track of those planes via the array, you’d have to come up with some other much more complex system to do so.
That said, the initCheckPlanes statement I agree with.
Ok... The code for checking the planes was written and rewritten and has resulted in good performance. I don't see any real reason to change it because you feel it is too complicated. You somehow have to cycle through each of the planes in the camera checks regardless of which one is checked first, so if you don't keep track of those planes via the array, you'd have to come up with some other much more complex system to do so.
As for the cycling through planes, it can be done with about the same amount of code that is done currently. What I'm talking about is why every bounds object out there have to duplicate code which helps to iterate through six values ?
As for the complication of solutions
Current code
int planeCounter = FRUSTUM_PLANES-1;
int mask = 0;
int rVal = INSIDE_FRUSTUM;
for (; planeCounter >= 0; planeCounter--) {
mask = 1 << (bound.getCheckPlane(planeCounter));
if ( (planeState & mask) == 0) {
int side = bound.whichSide(worldPlane[bound.getCheckPlane(planeCounter)]);
if (side == Plane.NEGATIVE_SIDE) {
//object is outside of frustum
if (planeCounter != FRUSTUM_PLANES-1) {
int i = bound.getCheckPlane(FRUSTUM_PLANES-1);
bound.setCheckPlane(FRUSTUM_PLANES-1, bound.getCheckPlane(planeCounter));
bound.setCheckPlane(planeCounter, i);
}
return OUTSIDE_FRUSTUM;
} else if (side == Plane.POSITIVE_SIDE) {
//object is visible on *this* plane, so mark this plane
//so that we don't check it for sub nodes.
planeState |= mask;
} else {
rVal = INTERSECTS_FRUSTUM;
}
}
}
Example code with single value
int mask = 0;
int rVal = INSIDE_FRUSTUM;
for (int planeCounter = FRUSTUM_PLANES; planeCounter >= 0; planeCounter--) {
if ( planeCounter == bounds.getCheckPlane())
continue; // we have already checked this plane at first iteration
int planeId = (planeCounter == FRUSTUM_PLANES) ? bounds.getCheckPlane() : planeCounter;
mask = 1 << (planeId);
if ( (planeState & mask) == 0) {
int side = bound.whichSide(worldPlane[planeId]);
if (side == Plane.NEGATIVE_SIDE) {
//object is outside of frustum
bounds.setCheckPlane(planeId);
return OUTSIDE_FRUSTUM;
} else if (side == Plane.POSITIVE_SIDE) {
//object is visible on *this* plane, so mark this plane
//so that we don't check it for sub nodes.
planeState |= mask;
} else {
rVal = INTERSECTS_FRUSTUM;
}
}
}
I don't think that latter code is more complicated - it is about the same (you get a bit of extra logic at start of loop but don't have to perform swapping on bounds object in middle). You don't have to put extra array in every bounds object out there, perform initialization and put 0,1,2,3,4,5 there and have a bit more clear interface.
This is not a speed change - speed should be exactly the same. Memory usage should be a bit smaller (probably 40 bytes less and one object less per geometry object), but this probably won't make a big difference. Main reason is simplicity - avoiding to encumber model objects with internals of implementation of particular algorithm more than it is needed (and requiring them to preserve 0-5 array for iteration IS a encumbering).
The system i describe is very easy to setup and is much more customizable. Intersections of BoundingVolumes and intersections of Triangles are two different task so should have their own routines. Usually, just one of the two will do and the other is unneeded computation. Collision detection will work just like it does now for most things, but instead of refrencing an outside, 3rd party class to do the work, you refrence the actual object that is actually doing the work. Code is also more human readable.
Does he just want to know if two spatial's bounding volumes intersect, or is he wanting to know which spatials of the two sets of spatials (IE the two nodes intersect).
very descriptive. I can tell the two bounds are being tested for intersection
Spatial a,b;
// recurses through a and b's children if they are nodes.
a.findBVIntersection(b,new IntersectionTest(){
public void onIntersection(Spatial a,Spatial b){
System.out.println("The spatial " + a.getName() + " intersects with spatial " + b.getName());
}
}
Very easy to tell what's going on with that code
Spatial a,b;
// every leaf of A against every leaf of B
a.findTriIntersections(b,new CollisionDetection(){
public void onCollision(TriMesh a,int trianglea,TriMesh b,int triangleb){
//... color A's ath triangle blue and B's bth triangle red
}
That's the system I imagine for the best blend of speed and customizability.
I like all of your suggestions there cep. Just one little clarification on this code:
Spatial a,b;
// recurses through a and b's children if they are nodes.
a.findBVIntersection(b,new IntersectionTest(){
public void onIntersection(Spatial a,Spatial b){
System.out.println("The spatial " + a.getName() + " intersects with spatial " + b.getName());
}
}
The onIntersection method's spatial b. Is that the same as Spatial b outside the method? If so, what would happen if Spatial b (the outside) is a node? would Spatial b (argument) be the actual Geometry that Spatial a has collided with, or would it be the same as Spatial b (outside) ?
Sorry about that. Using the letters a and b for both the onIntersect call and making the main call was confusing. They don’t have to be the same spatials. Nodes would recurse calls to children.
Check out the code review. There’s a change going in on how it works. But if you just want to see if any triangle/triangle collisions occur, there is a very fast method for just that in TriMesh. It’s faster because it returns after the first collision.
If I sound slightly perturbed it is because we get a handful of comments like this every week or so on small things that in a .7 state are not very relevant. I'd love these smart developers - and they all seem fairly decent and well meaning - to instead offer to code parts of the system we are still missing... (Shadows for example, or S3TC support) NOT pick through code that is already doing a decent job.
Just to let you know why. Before adding something to system, I have to get a feel of it - both how it works and how it is coded. To do this, I look through the code, and try to understand the flow. Sometimes I hit the parts which are puzzling to me - and turn out to be some kind of legacy code which nobody will fix ever as it already 'works'. In many cases, fixing it won't probably do any noticeable difference, both in speed and memory. Only difference here is that somebody in future, doing the same thing (trying to understand the code), will have a one problem less.
Here the problem was, that I understood that you cache last plane which rejected the object, but was not able to understand why you cache 5 others. It looked like you cache next ones in order of probability of rejection, but code doesn't work this way. So I thought that it is probably an error in coding - somebody wanted to cache all six in order of decreasing probability. But at the same time I thought that benefit of caching more than one is probably so neglible, that as well you can stick to caching only one plane, instead of rearranging entire array. Thats all.
I have started to focus on bounding objects because I have done some work with them for xith3d and could probably directly port some code here .
What do you think about adding extra method for ray intersection, which would return point of intersection (and maybe normal) ? It comes close to collision system, which will have to be outside bounding objects anyway, but ray/bounding volume check is probably helpful and easy enough to be treated especially. It would be especially helpful for picking, where you would need to sort objects in ray according to interesection points.
As I understand, BoundingVolumes are already going away from extending TriMeshes. Unfortunately I see another problem here - use of static temporary variables. This means that there can be only one thread using these objects. While it is ok for renderer (you can guarantee this), bounding objects are generic enough to be used also in other parts - for example on server. Even such simple thing as displaying some 'loading' screen while creating new scene on client in another thread can cause problems. If it is ok, then I think it should be stressed very strongly in both bounding objects javadocs and in jme tutorial, that apps using jme should use only one thread to interact with any of jme classes - regardless if it is live scenegraph, or just reusing few helper classes.
Unfortunately I see another problem here - use of static temporary variables. This means that there can be only one thread using these objects.
It is still possible to use multiple threads. All you need to do is make shore that only one thread is accessing the JME's classes at a time. although this may not work well when you need a server it works fine for a gui. Awhile ago someone on the form segeted having only one thread downing the modification and another thread pushing the modifications needed into qwere.
As I understand, BoundingVolumes are already going away from extending TriMeshes. Unfortunately I see another problem here - use of static temporary variables. This means that there can be only one thread using these objects. While it is ok for renderer (you can guarantee this), bounding objects are generic enough to be used also in other parts - for example on server. Even such simple thing as displaying some 'loading' screen while creating new scene on client in another thread can cause problems. If it is ok, then I think it should be stressed very strongly in both bounding objects javadocs and in jme tutorial, that apps using jme should use only one thread to interact with any of jme classes - regardless if it is live scenegraph, or just reusing few helper classes.
jME isn't thread safe any more than Xith3D. Users can still use jME classes in multiple threads correctly, but it is the user's responsibility to syncronize functions that use the same class. This makes life on the heap much easier. What I'ld love of course, is a Thread local variable type
For what it's worth though, I found the caching thing strange too. When I was trying to learn jME, using an array seemed strange. I always wondered why bounding volume arrays were [6] instead of something like [Camera.NUM_PLANES] or whichever that variable was.
jME isn't thread safe any more than Xith3D. Users can still use jME classes in multiple threads correctly, but it is the user's responsibility to syncronize functions that use the same class. This makes life on the heap much easier. What I'ld love of course, is a Thread local variable type
AFAIK, in xith3d only problem is with live objects - just don't touch objects being rendered from outside of rendering thread. You could use all classes in other context, just not instances which happen to be in live scenegraph.
It is not possible to synchronize anything here - while I could synchronize everything on my part of code, renderer running in other thread could still mess with static variables.
I guess that solution here is just to put a big "SINGLE THREAD ONLY" sign and stop worrying about it - or copy/paste interesting classes into different packages and use them for non-rendering tasks.
)