We certainly accept patches, though we have to tread carefully with patches that break existing code. Sweeping changes should either be implemented with a new switch (like the TaskNew.py reimplementation of Task.py, for instance), or at least put on a CVS branch, until they can be safely integrated for everyone.
David
Only trying to push it further up the queue - I know you want it fixed, I just want it fixed sooner:-) However, adding a collide that accepts an OdeSpace would certainly be a good stop gap solution.
There are several reasons I put this ‘game’ together - partially to see how its done, partially so I can try out some AI. But its also to actually use Chicken for a level editor myself, so I can spot any missing features, and to show off that capability so other people know it exists/use it. Plus there is no ‘complete’ fps example for Panda, so I thought the community would appreciate such an example. These last two require it to work with a straight copy of Panda though, hence why I really want to avoid using PyOde or my own Panda version.
P.S. You already have the function - OdeUtil.Collide, its only because of the inheritance issue that I can’t use it. Just makes it doubly frustrating…
I’d love to see this fixed the real way too, I just can’t make major design decisions like this without permission from Disney.
If you wanted to do major restructuring development without Disney’s involvement, it would be fine to do it on a CVS branch. When you reached something that seems solid and a good improvement, we can work together to integrate your changes to the trunk. 
David
Hey, that’s not a bad idea. I’ll work on that, right after I finish collada animations.
If I added this odeNode stuff to panda, where would they have to be in the tree? In ode, or pgraphnodes, or …?
lethe, I can at least provide the OdeUtil.collide(space, space) method in the next bugfix release - it would be just a tiny addition. Would you need anything more than that?
I think specialized node types like OdeNode would be best placed in the ode directory, just as CollisionNode is in the collide directory.
David
From my point of view what I need is for the dCollide function, which is currently exposed as OdeUtil.collide, to accept a space as one parameter and a geom as another parameter. As that function can internally accept either a geom or a space for either parameter I would change it so the interface takes either - this should not involve changing any of the actual execution code, just what it accepts as inputs. For future compatibility that makes sense, as adding a collide specific to 2 spaces would make no sense in the future when spaces are geoms and the functionality exists elsewhere. (I could always use a space to space approach - I’ld just have to put the ray in a space all on its own.)
There are two other collision functions - dSpaceCollide and dSpaceCollide2. You already have the first one, dSpaceCollide, exposed in the OdeSpace interface, but not dSpaceCollide2, which you might want to add to the OdeUtil or OdeSpace interface. (It takes two spaces or a space and a geom, it then calls a callback for all potential collisions between the two.) SpaceCollide2 would, whilst less efficient, provide an alternative solution to my problem, as it would at least allow me to use the fast collision culling in ode to limit which dCollide’s I then try. I only need dCollide though - I just mention this for completeness, as then all of the collision functions would be exposed.
I’ve just checked in OdeUtil.collide2, which works the same like PyODE (implements dSpaceCollide2) and a temporary OdeUtil.spaceToGeom() function (it converts an OdeSpace into an OdeGeom) - be very careful with it. It will most likely be removed later when we make OdeSpace inherit from OdeGeom.
Thank you:-) I’ll be curious to see if there are any cases when spaceToGeom doesn’t work, though will obviously keep use to a minimum as you say.
Hey guys, I finally got a chance to start looking back into this ODE code. I hope you aren’t finding it too troublesome. The original goal was always to have it more integrated into Panda, so I’m glad there’s some noise about it now.
Two quick things:
)Currently the system is very much pass-by-value. I’m trying to remember if there was any pressing reason to do it this way, but nothing’s jumping out. I think it may have started out as a strategy to more easily manage/track the lifetimes of the internal ODE objects (a fool’s errand I now believe, see below $). Unfortunately it leads to problems like you’re seeing with the OdeSpace/OdeGeom separation. Due to this pass-by-value design, we’re only allowed to return concrete Ode objects from our functions like OdeSpace.getGeom(). Obviously this causes a problem when you want to set up any hierarchy of spaces, as lethe discovered. By moving to a reference-count design and using heap-allocated objects, we can simply return a OdeGeom* that can be subsequently downcast to an OdeSpace* (provided you’ve taken the proper precautions and know it’s really a space)
I propose that we transition our Ode classes to TypedReferenceCount objects and start returning pointers to dynamically allocated Panda objects. I also believe this will smooth the transition to any system like the one pro-rsoft’s proposing.
$ One thing that’s always bugged me about ODE is how difficult it is to know if an internal ODE object is “valid” at any given time. It doesn’t make any sense to try to have our Panda Ode objects exactly mirror the lifetime of the internal ODE object. In fact, it’s pretty much impossible when you have things like OdeWorld.destroy() which causes the destruction of all internal ODE objects out from under any associated Panda Ode objects. Take a look at odeTriMeshData.h to get an example of how troublesome it can be to manage data lifetimes across the Panda/ODE divide.
pro-rsoft, you’ve recently added some functionality to allow users to check this validity via automatic boolean conversion and the isEmpty() function. Do you have any ideas about how we might address that issue?
I didn’t see a branch for this yet, so I’ve created one in the panda source tree as a sandbox for this project. Here’s how I suggest getting/using it:
Let’s assume you have directory hierarchy similar to this
- top
- panda3d
- dtool
- panda
- pandatool
- direct
- anothercrazycoolprojectI would suggest creating a parallel tree so you’d have this:
- top
- panda3d
- dtool
- panda
- pandatool
- direct
- ode-dev
- dtool
- panda
- pandatool
- direct
- anothercrazycoolprojectThen update your ode-dev/panda directory with:
cvs up -r ode-developUse “cvs st” to verify that your files now have the branch sticky tag. Anyone with repository write access can now commit to that branch just as they would the trunk.
It’d work best if one person managed the branch, so as not to step on each other’s toes. Let’s figure out who that should be and if needed I can give some instructions on how to periodically pull in changes from the trunk to the branch.
Thanks, this will indeed help me.
Currently, the OdeGeom, OdeJoint, etc. classes remind me very much of NodePath. It’s a not-reference-counted wrapper around a PandaNode - you can have multiple NodePaths pointing to the same node, destroy them without affecting the PandaNode, etc. The one difference is that because ODE isn’t reference counted itself, we can’t just store a reference and it won’t be deleted. I think that’s the biggest problem with the current approach.
If you take a look at the “odecpp.h” and “odecpp_collision.h” in the ODE source, (your version of ODE might or might not have them) you can see they have their own C++ wrapper around the ODE functions and structures. For example, from the dGeom class:
class dGeom {
(...)
protected:
dGeomID _id;
public:
dGeom()
{ _id = 0; }
~dGeom()
{ if (_id) dGeomDestroy (_id); }
dGeomID id() const
{ return _id; }
operator dGeomID() const
{ return _id; }
void destroy() {
if (_id) dGeomDestroy (_id);
_id = 0;
}
(...)
dSpaceID getSpace() const
{ return dGeomGetSpace (_id); }
(...)
}Also, they have a dSpace class which inherits from dGeom.
It looks like they are just a bit closer to representing the lifetime (although they still provide a way to destroy the geom before the actual class destruction).
However, if you look at the getSpace() function, it still returns a dSpaceID. That’s one of the problems with the other approach of making the objects represent the exact lifetime - you will need to return the exact same OdeSpace object as you gave it. So even if we inherited from those classes, we would still have problems.
The solution for that would be to store a table for that with a pointer to the dBlahID’s and our own objects - we’d need to add some extra memory management smartness. (Also, with this approach, we do need to make the objects reference counted.)
I’ve played a bit with PyODE and noticed they handled these situations fine. I’ve looked in the source code and found this:
# Each geom object has to insert itself into the global dictionary
# _geom_c2py_lut (key:address - value:Python object).
# This lookup table is used in the near callback to translate the C
# pointers into corresponding Python wrapper objects.Surprisingly, it does look like they use my idea above.
If we chose this idea, we’d need to overhaul the entire layer and we might break people’s code - but I think most people have their code organized in a way that they only have one OdeWhatever pointing to a dWhateverID, so it wouldn’t be much of a problem.
EDIT: Thanks for creating the branch - I will do as you suggested.
I don’t know who should manage it, I’m afraid I lack some CVS experience for that.
I see the ode-develop as a normal tag and not a branch tag, however - how exactly would that work?
Whoops, try the same update command now. It should be a branch sticky tag now.
I can see how the lookup table would help keep one wrapper per object, but it doesn’t solve all of the lifetime issues.
Here are the two situations I see, assuming a single wrapper per internal object:
1 - The wrapper is destroyed because no one cares about it or needs it at the moment. The internal object is still valid and another wrapper will be created when needed.
2 - The internal representation is destroyed and the wrapper is not informed.
This would all be a non-issue if ODE exposed functionality to test dID validity. Maybe they do and I’m just missing it. That would eliminate the need for any lookup tables and potentially the need for reference counting on the wrappers (though there are other benefits to the ref counting model).
Well, my suggestion is not to make the Ode objects wrappers at all, but to let them represent the internal ODE object. When the Ode object is created, the dID is created. When it’s destroyed, the dID is destroyed. (This is how pyODE works, too, I think.)
Well, I think we do. If someone chooses to let the reference count become zero, he deserves to get the object destructed. Just like if you remove references to an Actor, the animation stops playing.
This is exactly what the odecpp builtin dGeom class does, too.
That would be a memory leak, because there won’t be a way to get it back anymore - I don’t see how, if the user wants it to persist, he shouldn’t remove the references to it.
So the pointers in the lookup table should be WeakPointerTo’s, I think.
As for your point 2, that is indeed true - but I’ve checked how PyODE does it. After deleting my references to the world and garbage collecting, neither the world nor its bodies get destroyed. This is because the bodies internally store a pointer to the world - and you still can get this world by calling getWorld or so.
This approach certainly makes sense, and would solve the issues with point 2 - we just need to store a reference to the OdeWorld in classes that depend on it. Then, if someone wants a world to be destroyed, he would first have to destroy all bodies in it.
It might also be a good idea to remove the destroy() methods if you decide - or at least make them do nothing.
It’s up to you if we really want to pursue this idea - it would bring the pandaode layer much closer to the ODE structures itself, but it does require a fair bit of restructuring and might even break current code.
I’ve been looking for a way to check whether a dID exists, but couldn’t find one. That would certainly solve the issues with the original approach (what we have now, with the wrappers.)
On a sidenote, I’ve created an OdeNode class inheriting from both PandaNode and OdeBody, but I’m getting this error:
Opt3-Linux/libpode_igate.cxx: In function ‘PyObject* Dtool_OdeBody_downcast_to_OdeNode_622(PyObject*, PyObject*, PyObject*)’:
Opt3-Linux/libpode_igate.cxx:9946: error: request for member ‘as_typed_object’ is ambiguous
/usr/local/panda/include/typedObject.I:110: error: candidates are: const TypedObject* TypedObject::as_typed_object() const
/usr/local/panda/include/typedObject.I:99: error: candidates are: TypedObject* TypedObject::as_typed_object()
/usr/local/panda/include/typedObject.I:110: error: const TypedObject* TypedObject::as_typed_object() const
/usr/local/panda/include/typedObject.I:99: error: TypedObject* TypedObject::as_typed_object()
make: *** [Opt3-Linux/libpode_igate.o] Error 1Is inheriting from TypedObject twice like this invalid, or so?
You can inherit twice from a class like TypedObject, but you have to disambiguate the inherited methods. This means you define methods in your class like this:
INLINE int get_type_index() const {
return OdeBody::get_type_index();
}
INLINE bool is_of_type(TypeHandle handle) const {
return OdeBody::is_of_type(handle);
}
INLINE bool is_exact_type(TypeHandle handle) const {
return OdeBody::is_exact_type(handle);
}
INLINE TypedObject *as_typed_object() {
return OdeBody::as_typed_object();
}
INLINE const TypedObject *as_typed_object() const {
return OdeBody::as_typed_object();
}Basically, every method that is inherited from TypedObject and might be called on a pointer of this type, has to be shadowed, and the method body needs to be an upcall to one inheritance path or the other. Since both inheritance paths amount to the same thing (TypedObject doesn’t have any of its own data members), it doesn’t matter which one you choose, but C++ makes you pick one or the other. (As opposed to Python, which picks one for you.)
Note that you cannot doubly-inherit from ReferenceCount; that would fail miserably, because then your object would have two different and competing reference counts.
David
Seeing that this is where all the ODE stuff is going on it seems the best place for bug reports:
The OdeUtil.collide method returns an OdeContactCollection from which you can get a bunch of contacts of type OdeContact. Each contact has a getGeom method, which returns an OdeContactGeom - it appears that this object contains all the useful information - i.e. it has getPos, getDepth and getNormal methods - problem is all these methods return 0 for getDepth or a vector of zeroes for the other two.
I presume they are meant to return where the collision happened and the surface normal at that point - as it is I’m not going to be adding bullet holes any time soon! I can’t even reliably sort collision points - I’m surprised I hadn’t noticed before but I sometime shoot the object behind the object I’m aiming at, which is really not good.
P.S. After writing the above I had that ‘do any methods return anything useful’ thought. It would appear not - the getGeom() method just produces a truck load of zeroes.
Ah-hah. I see what’s wrong. A small typo in an OdeContactGeom copy constructor - I’ve fixed it for the upcoming 1.6.1 release.
Also, I’ve phased out OdeContactCollection. When I added OdeCollisionEntry it ended up more powerful than the contact collection - OdeUtil.collide returns an OdeCollisionEntry now (and I’ve added the [] operator so I don’t think it breaks).
Also, OdeCollisionEntry.getContactGeoms returns a true python list that you can easily sort using standard python sort and a lambda.
Sounds good - I look forward to 1.6.1:-) I’m not sure what you mean about using OdeCollisionEntry rather than OdeContactCollection though - the second has a lot more capability, especially when collide is being applied to a space and can hence return collisions between multiple possible bodies, not to mention all the other info. But then I’m comparing with the current classes in the API, which doesn’t include a OdeCollisionEntry.getContactGeoms for one, and those two could certainly be merged. But as long as your sure you haven’t lost any functionality then I’m happy!