rcBuildContext::getBuildTime should be const

[GoogleCodeExporter]

rcBuildContext::getBuildTime function should be a const function.

To be honest, I would ditch the idea of timers in rcBuildContext and simplify 
the interface to:

struct rcBuildContext
{
   virtual void resetLog() {}
   virtual void log(const rcLogCategory /*category*/, const char* /*format*/, ...) {}

   virtual void resetBuild() {}
   virtual void startBuild(const rcBuildTimeLabel /*label*/ ) {}
   virtual void endBuild(const rcBuildTimeLabel /*label*/ ) {}
};

If a derived concrete implementation wants to track build times, then great - 
they can easily do that via this interface. It will simplify the Recast code - 
cut down on the number of virtual functions called and not prejudice the way 
derived classes should implement time functions. At the end of the day, Recast 
itself is not intrinsically interested in build times (it's not important to it 
getting the job done)...only a client is interested in these things. With the 
above interface you also get good symmetry (perhaps not the right description?) 
between the start and end points of a build function...which "feels" right.

Original issue reported on code.google.com by armstron...@gmail.com on 24 Aug 2010 at 8:00

[GoogleCodeExporter]

I think I agree on that :)

Original comment by memono...@gmail.com on 24 Aug 2010 at 8:05

[GoogleCodeExporter]

Actually, I'd do the following (taking into account issue #111):

note: This could be done just for the "build" functions as logging is called 
much less frequently and doesn't impact build performance.

struct rcBuildContext
{
   void callLogFunctions( bool call ) { callLogFuncs = call }
   void resetLog() { if (callLogFuncs) doResetLog(); }
   void log(const rcLogCategory category, const char* format, ...) { if (callLogFuncs) { va_list args; va_start(args, format); log(category,format,va_args); va_end (args); }

   void callBuildFunctions( bool call ) { callBuildFuncs = call }
   void resetBuild() { if (callBuildFuncs) doResetBuild(); }
   void startBuild(const rcBuildTimeLabel label ) { if (callBuildFuncs) doStartBuild(); }
   void endBuild(const rcBuildTimeLabel label ) { if (callBuildFuncs) doEndBuild(); }

protected:
   // Client overrides these functions:
   virtual void doResetLog() {}
   virtual void doLog(const rcLogCategory /*category*/, const char* /*format*/, va_list /*arg*/) {}

   virtual void doResetBuild() {}
   virtual void doStartBuild(const rcBuildTimeLabel /*label*/ ) {}
   virtual void doEndBuild(const rcBuildTimeLabel /*label*/ ) {}

   bool callLogFuncs;
   bool callBuildFuncs;
};

Original comment by armstron...@gmail.com on 24 Aug 2010 at 8:16

[GoogleCodeExporter]

Getting better :)

That's pretty much in line with recommendations of Mr. Bloom too.
http://cbloomrants.blogspot.com/2010/07/07-26-10-virtual-functions.html

I'm against having the varargs stuff in a header, but other than that it looks 
good. I might even bend over to passing it as a reference too.

Original comment by memono...@gmail.com on 24 Aug 2010 at 8:27

[GoogleCodeExporter]

>>I'm against having the varargs stuff in a header
Fair enough - I felt dirty adding varargs in to my code example...that varargs 
stuff looks *nasty* :) 

For performance reasons the non-virtual functions do need to be inline, but as 
the logging functions are not performance critical I guess it'd be ok for the 
implementation to be outside the header.

Original comment by armstron...@gmail.com on 24 Aug 2010 at 9:02

[GoogleCodeExporter]

P.S. rcBuildContext also needs a virtual destructor

Original comment by armstron...@gmail.com on 24 Aug 2010 at 10:11

[GoogleCodeExporter]

BTW: You could use a similar "bool callVirtualFunctions" technique on 
dtQueryFilter. Instead of having the DT_VIRTUAL_QUERYFILTER define. Just an 
idea. It would make the code a bit tidier :) Failing that, how about:

#define DT_VIRTUAL_QUERYFILTER virtual

...and use that in front of the function declarations - instead of repeating 
the function twice:

DT_VIRTUAL_QUERYFILTER bool passFilter(const dtPolyRef ref,
                    const dtMeshTile* tile,
                    const dtPoly* poly) const;

Original comment by armstron...@gmail.com on 24 Aug 2010 at 10:25

[GoogleCodeExporter]

Fixed in R206.

Original comment by memono...@gmail.com on 24 Aug 2010 at 6:00

[GoogleCodeExporter]

Original comment by memono...@gmail.com on 24 Aug 2010 at 6:01

• Changed state: Fixed