Strangenesses in ShapeBase::notifyCollision

#1 You didn't want to know this, but if you invoke a method on a null pointer the method will run just fine but "this" will be null.

#2 Eheh, yeah, I just don't see the chain of circumstances that would cause that to happen in this case. Could just be a conservative precautionary check of course.

#3 Joel,

I agree, case #1 seems a bit strange :))

Here's my guess for #2.

The first lines check the safeThis pointer is still a pointer (fair enough).

The next lines call a method that could actually delete the safethis pointer (maybe de-ref it and it delete's itself?)

third lines check its valid again.

One thing that just struck me, it doesnt use the safeThis pointer in the second set of code :))

Stumped.

Phil.

Lets face it though, if you wrote a whole boatload of code, left it for a year, I'm certain you'd find some funnies in it.

#4 Oh, certainly; I won't be letting anyone around here take a look at the source code to the last million-lines project I was involved in. No value judgement implied by these posts. :-) I just have a vested interest in making sure all this stuff works.

#5 The last million line project I was on had most of the interesting stuff ripped out after I left and was then open-sourced (possl.org). Hmm, not too dissimilar;)

#6 Soudns like the notify collision is correct. The point of the notifyCollision list is to weed out collisions so that the notify script is only called once per time out value (per object). This was basically a performance decision, so that the callback isn't invoked every single tick if your standing on something.

I would say Phils is correct on #2. The onCollision method invokes a script callback, which basically means anything can happen, including the object being deleted.

#7 The collision stuff works, absolutely, as long as you process the list on the same time you created it. I'm just saying that the existence of CollisionTimeoutValue would seem to imply that you should be able to process the list later, but that doesn't appear to be the case with the code the way it is written.

--

BTW, if you don't mind satisfying my curiosity... do those double safeThis checks look like duplicate code to you too, or is it actually performing some arcane function that I've overlooked?

#8 Joel you're not being clear what you're asking, but in the code:

SimObjectPtr<ShapeBase> safePtr(ptr->object);
         SimObjectPtr<ShapeBase> safeThis(this);
         onCollision(ptr->object);
         ptr->object = 0;

         if(!bool(safeThis))
            return;

         if(bool(safePtr))
            safePtr->onCollision(this);

         if(!bool(safeThis))
            return;

First: The "cast to bool" idiom is one of the stupidest idioms ever to use in C++ because it implies overriding the cast to bool operator which in turn leads to C++ automatic mis-casting as the compiler attempts to resolve ambiguities in overloaded functions. And other problems. Just don't go there. safeThis->isValid() is both clearer and safer. What, for instance, is the difference between "if(!bool(safeThis))" and "if(!safeThis)"? 99% of programmers can't tell you the difference coherently. And no one can tell you the practical difference without carefully examining the classes involved, which is just wrong.

In this particular case, Tim mentions the fact that onCollision can invoke a script which could delete the underlying objects. Perhaps the code once responded to this case, or perhaps he assumes too much about its authors. Or perhaps this C++ is above my head, which is always possible;)

As near as I can tell, since safePtr and safeThis are automatic classes, bool(safePtr) and bool(safeThis) must be syntax errors unless they automatically invoke

inline operator T*() const { return static_cast<T*>(mObj) ? static_cast<T*>(mObj) : 0; }

and then implicitly perform "(T*)mObj != 0". Especially in the case of safeThis which we know is a ShapeBase and ShapeBase does not override or virtualize "operator bool" and we can go traipsing through the whole class hierarchy without finding anything that does (I think).

mObj is a private instance variable of template SimObjectPtr which--it is important to note--in this case has no super-classes or sub-classes. Given that and the fact that the address of safePtr and safeThis themselves are never given out, I don't see how the literal value of mObj can change, so I don't see how the various bool(safeThis) and bool(safePtr) can start returning new values.

I mean, I see why it's important to notice that the object we're standing on has been deleted, I just don't see how it gets detected.

Of course, I'm so inept I can't figure out in what circumstances the code

operator T*() const { return static_cast<T*>(mObj) ? static_cast<T*>(mObj) : 0; }

returns a different result than

operator T*() const { return static_cast<T*>(mObj); }

, so just color me ignorant.

#9

Quote:Joel you're not being clear what you're asking

Sorry about that. :-) If you're referring to the "#2" case, I'm just saying that the first safeThis check seemed unnecessary. notifyCollision is never called on an object pointer by some other object; instead, an object calls its own notifyCollision member function, in the midst of doing a whole lot of other code. So it seems... unlikely... that "this" could be NULL before the call to onCollision. The check after onCollision is fine and dandy, as we don't want to continue checking for collisions or indeed running any code at all if onCollision deletes this object, but the one before it seemed redundant. No big deal, and not a Deep Insight, just something I was commenting on en passant.

As for how the cast to bool ends up referencing the value of mObj, I had roughly the same reaction as you, but didn't worry about it overmuch. This comment though:

Quote:Given that and the fact that the address of safePtr and safeThis themselves are never given out, I don't see how the literal value of mObj can change, so I don't see how the various bool(safeThis) and bool(safePtr) can start returning new values.

is something that hadn't occurred to me, and does seem worrisome.

#10 Ah, here's what's going on. When you create a SimObjectPtr with an object pointer as the constructor argument (like the creation of safeThis), the constructor invokes SimObject::registerReference with the address of the mObj field as argument.

If the scripting later removes the object, it will cause the following sequence of invocations: SimObject::deleteObject -> SimObject::unregisterObject -> SimObject::processDeleteNotifies. Among other things, processDeleteNotifies goes through all the references registered on the object and sets their associated pointers (mObj) to NULL.

#11 The sad thing is that exact constructor has been displayed on my monitor overnight. For some reason I thought it was doing standard auto-pointing. The magic of C++!

Still, the correct call is to "safePtr.isNull();"

I also can't figure out what "mObj = const_cast(static_cast(rhs));" does that is better than "mObj = rhs.mObj;" There isn't a "SimObjectPtr::operator const T*()", so instead it is (isn't it?) invoking "SimObjectPtr::operator T*()". Or is it invoking "(T* SimObjectPtr::operator->())->operator const T*()"? It's hard to trace directly because the including function, "SimObjectPtr& SimObjectPtr::operator=(const SimObjectPtr& rhs)" is never used.

#12 "As for how the cast to bool ends up referencing the value of mObj..."

My worry wasn't "How" but "Whether".

If T declares "T::operator bool() const" then does the compiler have the option of invoking "(T* SimObjectPtr::operator->(safePtr))->operator bool() const)"? Because if it does, that resolution does not require any implicit casts, so it would be preferable to implicitly casting safePtr to "T*" so that it could then cast it to bool.

I guess we're really starting with the copy constructor "bool::bool(const bool&)" so maybe there's an implicit cast anyways...I suppose it's back to the ARM for me.

#13 I agree the bool cast is lame. Are we all clear on the rest of it though? Besides the bool cast, it looks fine to me :) Having objects deleted out from under you is never pleasent.

#14 One last thing about the safeThis checks. Just above the notifyCollision code that I included in my original post, there's this code:

SimObjectPtr<ShapeBase> safePtr(ptr->object);
         SimObjectPtr<ShapeBase> safeThis(this);
         onCollision(ptr->object);
         ptr->object = 0;

The onCollision call for the colliding object doesn't get informed about "this" object. But, you know, the scripting could in theory do anything it wants in that call (like delete all objects in the game), so that is probably why the first safeThis check exists.

(BTW, the fact that the code sets ptr->object to 0 looks awkward at first, since it afterward calls a member function of safePtr, but really setting ptr->object to 0 is just marking this particular collision as "done"; it doesn't affect the value of safePtr.)

So... it turns out that the SimObjectPtr manipulation stuff looks fine, with the possible exception of making frowny-faces at the bool cast.

That was a fun little exercise. :-)

---

Quote:Are we all clear on the rest of it though?

Well, not to belabor a point, but although I was curious about what was up with the SimObjectPtrs, the main thing is that I was wondering why the whole timestamp rigmarole exists, especially the existence of a "timeout value", if the algorithm is only functional (AFAICT) when you process the queue on the same time as you create it.

Currently in the code, notifyCollision is always called on the same time as queueCollision, so it's functional, just doing what appears to be some useless work.

So that there's no confusion: Generally, if I post here rather than the Bugs forum, it's about code that looks iffy but I'm not entirely sure that I've understood it correctly. It's not an OMG BUG FIX IT QUICK sort of thing, just trying to figure stuff out. When I see code that looks like it's doing the right thing, I tend to assume that I'm understanding it, so I don't post about that sort of code. :-)

#15 This list is providing two functions: a list of new collisions that need to be notified, and a list of old collisions that need to be ignored. The timestamp compare is just trying to figure out which entries just got added without using a flag.

I guess you could split that into two lists, new collisions to notify, and old collisions to ignore. After the notify you add all the new collisions to the ignore list.

The current list gives you a "two for one", at the expense of looping through old collisions to figure out which ones need to be notified. It may be doing (occasionally) a little extra work, but it doesn't seem useless work to me, unless I'm overlooking something?

#16 The timestamp does let you avoid calling onCollision on old collisions, but at first glance I didn't see why you needed to be doing timestamps and keeping old collisions around at all, since collisions are performed on the same tick that they are queued.

Think I got it now though. I took a closer look at the queueCollision code, and the key thing I overlooked before is that it does an early return if it finds an old entry for the same object stamped less than CollisionTimeout ticks ago. So, the central reason for doing all of the timestamp-CollisionTimeout-keeping-old-collisions-on-the-list is that you want to avoid doing more than one collision with the same object within that period of time. Makes sense.

All happy now I think!

#17 ok :) Better source level documentation would make this easier. Though after typing up all these posts, I'm not sure I feel up to do it all over again in the source!