Bugs in ODE system (with fixes)

Hi all,

I made a thirdparty package for ODE and tried to compile Panda3D with ODE support, but I ran across four issues:

*1): I got this error from interrogate:

          *** Error in thirdparty/ode/include/common.h near line 331, column 31:
          syntax error, unexpected INTEGER, expecting '}'
Error parsing file: 'odeAMotorJoint.h'

This is the piece of code, the corresponding line bold:

D_ALL_PARAM_NAMES_X seems to be a #define:

#define D_ALL_PARAM_NAMES_X(start,x) \
dParamLoStop ## x = start, \
..etc..

That’s where the error seems to be. I think interrogate just can’t chew the ##.
I worked around it by manually appending the 2 and 3 to dParamLoStop, like dParamLoStop2, but it would be kewl if interrogate would support ## concatenation.

*2) Now I worked around the first error, I bumped into this error:

In file included from built/tmp/libpode_igate.cxx:38:
panda/src/ode/odeHeightFieldGeom.h:123:32: error: odeHeightfieldGeom.I: No such file or directory

panda3d.cvs.sourceforge.net/pand … a/src/ode/
The file does indeed not exist. Could this be fixed? Does it indeed not exist and the include line needs removed, or is a file missing at CVS?
I’ve just commented out the include line for now.

*3) There were some other errors, but they all seemed to be caused by one file. Looking at odeConvexGeom.I, its creator seemed to be a bit low of coffee when creating it. Here’s the corrected version, that actually does compile:
dpaste.com/45206/plain/

*4) Everything compiles fine… but when linking to libpanda.so, I get tons of ‘multiple definitions’-errors. It appears to be only for the classes that contain the word “Joint”.
Click here for the build log.

–pro-rsoft

Oh, I should have warned you - when making a thirdparty package, we try very hard not to edit the header files for the thirdparty package. If you edit the header files, then every time you update to a more recent version of ODE, you have to re-edit the header files. It’s too error-prone.

Instead, we try to put #defines into the panda side of things to disable the troublesome code, or put a dummy header into parser-inc.

1. Interrogate does indeed support the token-pasting operator (##). Not sure what the precise problem here is.

2. odeHeightFieldGeom.h is part of a work-in-progress. You shouldn’t attempt to compile it.

3. odeConvexGeom.I is also part of a work-in-progress. You shouldn’t attempt to compile it either.

In general, not all of the source files in panda/src/ode are ready to be compiled. You can tell which files are legitimate and which are still experimental by examining the Sources.pp file, which lists only the legitimate files.

For your link errors, it certainly appears that you are getting two copies of some of the .cxx files linked in with your library. Are you sure you haven’t added something twice somewhere?

David

I’ll try to leave the headers intact. But – how would I solve the ## problem then? How would would I do it using the method you suggested, Josh?

work-in-progress? That was the first commit and it was 14 months ago… Anyway, it contains erronous code and (imho) it could do no harm to correct it…

OK, I won’t compile those then, but how would I tell that to makepanda? Should I manually copy the wanted headers instead of using CopyAllHeaders, or does makepanda provide an option for it?

Anyone? Is there an option in makepandacore to exclude files or so?

I assume that you’ve added some lines to makepanda to compile the ODE stuff, and I assume that you copied-and-pasted those lines from some other directory. If that’s the case, then the lines you cut-and-pasted contain something like this:

IGATEFILES=GetDirectoryContents('panda/src/express',["*.h", "*_composite.cxx"])
TargetAdd('libexpress.in', opts=OPTS, input=IGATEFILES)

Ie, scan the directory to find all the dot-h and composite-cxx files, and then pass those to interrogate. It’s a straightforward matter to insert:

IGATEFILES.remove("foo.h")

or something like that.

Thanks, that worked. I got it to compile now, but I still got edited headers.
Could you please explain me how to fix the interrogate issue, so I won’t have to edit the ODE header files?

Well, there are many possibilities. One is that the macroexpansion of that header file contains something nonstandard that interrogate doesn’t understand. Another is that interrogate genuinely contains a bug. I can’t really tell what’s the truth based on what you’ve shown - there’s not enough shown to derive the macroexpansion and determine whether or not it’s syntactically correct. I think you should run that source file through the C++ preprocessor and see what that enum looks like after expansion. That will make it easier to tell what interrogate is barfing on.

The file is ode/common.h, line 345.

This is how it looks:

/* standard joint parameter names. why are these here? - because we don't want
 * to include all the joint function definitions in joint.cpp. hmmmm.
 * MSVC complains if we call D_ALL_PARAM_NAMES_X with a blank second argument,
 * which is why we have the D_ALL_PARAM_NAMES macro as well. please copy and
 * paste between these two.
 */

#define D_ALL_PARAM_NAMES(start) \
  /* parameters for limits and motors */ \
  dParamLoStop = start, \
  dParamHiStop, \
  dParamVel, \
  dParamFMax, \
  dParamFudgeFactor, \
  dParamBounce, \
  dParamCFM, \
  dParamStopERP, \
  dParamStopCFM, \
  /* parameters for suspension */ \
  dParamSuspensionERP, \
  dParamSuspensionCFM, \
  dParamERP, \

#define D_ALL_PARAM_NAMES_X(start,x) \
  /* parameters for limits and motors */ \
  dParamLoStop ## x = start, \
  dParamHiStop ## x, \
  dParamVel ## x, \
  dParamFMax ## x, \
  dParamFudgeFactor ## x, \
  dParamBounce ## x, \
  dParamCFM ## x, \
  dParamStopERP ## x, \
  dParamStopCFM ## x, \
  /* parameters for suspension */ \
  dParamSuspensionERP ## x, \
  dParamSuspensionCFM ## x, \
  dParamERP ## x,

enum {
  D_ALL_PARAM_NAMES(0)
  D_ALL_PARAM_NAMES_X(0x100,2)
  D_ALL_PARAM_NAMES_X(0x200,3)

  /* add a multiple of this constant to the basic parameter numbers to get
   * the parameters for the second, third etc axes.
   */
  dParamGroup=0x100
};

This is how it looks preprocessed:

enum {
  dParamLoStop = 0, dParamHiStop, dParamVel, dParamFMax, dParamFudgeFactor, dParamBounce, dParamCFM, dParamStopERP, dParamStopCFM, dParamSuspensionERP, dParamSuspensionCFM, dParamERP,
  dParamLoStop2 = 0x100, dParamHiStop2, dParamVel2, dParamFMax2, dParamFudgeFactor2, dParamBounce2, dParamCFM2, dParamStopERP2, dParamStopCFM2, dParamSuspensionERP2, dParamSuspensionCFM2, dParamERP2,
  dParamLoStop3 = 0x200, dParamHiStop3, dParamVel3, dParamFMax3, dParamFudgeFactor3, dParamBounce3, dParamCFM3, dParamStopERP3, dParamStopCFM3, dParamSuspensionERP3, dParamSuspensionCFM3, dParamERP3,




  dParamGroup=0x100
};

The error message occurs at the newline character of the D_ALL_PARAM_NAMES_X(0x100,2) line.

Hm. We really need a way to do subdirectories in the parser-inc directory.

No clues to fix this? I would have no clue how, someone would need to tell me.

Ah, I see the bug: the interrogate pre-processor doesn’t expect you to be token-pasting with an integer; it expects an alphanumeric string instead. So it ignores the token-paste operation.

I’ve just checked in a simple fix (in dtool/src/cppparser) that ought to address this. Though I haven’t tested the fix, it’s simple enough that my confidence level is high.

We do. There are lots of files in there that already install into subdirectories.

David

Ah, thanks! But, now I’ve run into another error (that is either caused by a change in ODE, which I upgraded to 0.9, or a change in interrogate):

        *** Error in /home/oldpro/projects/panda3d/thirdparty/linux-libs-a/ode/include/ode/odemath.h near line 294, column 22:
        syntax error, unexpected KW_VOID, expecting '{' or ';' or ':' or '='

This is the corresponding code:

touching parser-inc/ode/odemath.h didnt work.

This code is using the nonstandard keyword “__inline”. You can simply tell interrogate to ignore this new keyword, by adding “-D__inline” to the interrogate command line.

David

Oh yeah, that worked. Thx alot, I’m finally beginning to really understand how interrogate works.
But, how would I do that in makepanda, I would have to add new functionality then? Or is there a better way than just putting it somewhere in the makepanda code? I found DefSymbol, but that only gives things like “-Dfoo=bar”, not simply “-Dfoo”.
Or, can I just add it to the default options, so it will be defined for all igates?

Oh, and two other questions:
(1) would panda’s ode dir belong linked in into libpanda.so, or as a library on its own (just like libpandaphysics.so)
(2) is it OK if ODE gets committed to CVS as soon as I’ve tested it or should we wait till after 1.5.3 is released (so it could be in 1.6.0).
I ask this because usually x.x.3 releases are quite stable and therefore not much big features are being added usually.

PS. This is basically all that remains – the rest is mostly done, I got libs for linux and windows. Thanks to neighborlee I also got a linux-64 build.

“-D__inline” is the same thing as “-D__inline=”. So you can add it to DefSymbol.

(1) I believe we compile panda’s ode module as a separate DLL, called libpandaode.

(2) In general, it’s OK to commit experimental code to the trunk while a branch is pending. Josh will be able to pull in just the changes that are relevant to the 1.5.3 branch. We do this as a matter of course while developing Panda; otherwise, we’d have to hold up all development while we’re in the middle of a publish cycle. Difficult to do, since we are simultaneously making publishes for the CMU release, as well as for our own Toontown and Pirates games, so there’s almost always a publish cycle going on one way or another.

David

(0) Thanks. Well, I just saw DefSymbol does a bit more than just adding it to the interrogate options, so I just added it to the default options of calling interrogate, is this OK? I guess if __inline is a nonstandard symbol it won’t do any harm in setting it in each interrogate call?

(1) OK, thats fine, I’ll put that in makepanda. I just saw there is already a metalibs/pandaode, that will make stuff a bit easier for me.
But, I just can’t get genpycode to like my libpandaode library:

ImportError: dynamic module does not define init function (initlibpandaode)

Yes, I did link the metalibs/pandaode in, as far as I know.

(2) Ah, thanks for the clear explanation. So, besides this, I’m done. I’ll just run a test build on windows (again, I spent half my weekend compiling), and will commit. I guess Josh will then decide in which release it’s going to be.

Right. We have lots of such symbols already set on every interrogate call.

I’m not sure how makepanda handles this, but genPyCode is a generated script by ppremake. When ppremake runs, it puts the libraries named by GENPYCODE_LIBS into the genPyCode script (you can see the default definition of this variable in dtool/Config.pp). Presumably makepanda has some similar mechanism.

David

Oh, you can also name additional libraries at runtime, e.g.

genPyCode libpandaode

David

Right, but I found a list with all libraries in direct/src/ffi/jGenPyCode.py, I just added libpandaode there, resulting in this error:

ImportError: dynamic module does not define init function (initlibpandaode)