dropecho / desteer

desteer is a steering behaviors library written in C++. Its meant to be easily extensible and very clean.
13 stars 1 forks source link

SetMobile should call SetMobile in owned sub-behaviors #8

Closed dennisferron closed 12 years ago

dennisferron commented 13 years ago

Using SimpleSteeringController, started getting intermittent segfault in FleeBehavior::Calculate; called by EvadeBehavior, called by HideBehavior. The problem is that behaviors (such as HideBehavior and in turn FleeBehavior) are creating child or sub-behaviors but not passing the mobile entity down to them when it is set. Compounding the error is the fact that the constructor of FleeBehavior should set the _mob member to NULL since it isn't passed in the constructor, but instead it remains uninitialized - sometimes it points to address 0x30 or 0x40, sometimes it may point to valid memory or even point to a different entity by accident or anything else. (Ben, I think this may be responsible for the random weird steering you're seeing sometimes in the ExampleWithIrrlicht, or responsible for the steering getting "stuck" other times.)

We should probably decide on a standard whether the mobile entity is or is not to be passed in the constructors of behaviors (some do, some don't, some get passed NULL). I am wary of any scheme that requires you to remember to call a second function (e.g. SetMobileEntity) to set an essential member (essential because Calculate() bombs in some behaviors without the _mob member being set). However I comprehend the difficulty of needing to have all an object's required inputs be present at object creation time; sometimes a game engine's architecture really does require the ability to create the object in one place and finish initializing it elsewhere later. (Although I must point out this specifically and egregiously violates the C++ RAII principle.)

If we cannot or do not want to pass the IMobileEntity* in all the behavior constructors, we should at least adhere religiously to setting all uninitialized members to NULL in the constructors and checking them for NULL before they are used in a method, or at least put a debug assert for that in the methods they're used in.

And in all cases, if SetMobileEntity is designed to allow you to change the _mob member after it has already been set to something else (is that intentional?) then SetMobileEntity in a behavior should recursively call SetMobileEntity for child behaviors.

I faced a similar issue in LikeMagic with the mark() function - needing to mark() child objects so the garbage collector knows they are not garbage. I kept forgetting to mark() some owned child objects. I decided it was too unreliable to rely on just remembering to do that everywhere, so I implemented a class called MarkableObjGraph that tracks child objects and marks them automatically. A similar solution here would be: create a base class for behaviors that keeps track of child behaviors owned by each behavior, and which forwards calls like SetMobileEntity recursively down the object hierarchy.

dennisferron commented 13 years ago

Actually it appears EvadeBehavior is the only one not calling SetMobile for its owned sub-behavior.

vantreeseba commented 13 years ago

It must have been when I changed from the singular behavior functions to encapsulating them in classes.