C++ Question Pointer Problems

[escapetomsfate]

Ok, I'm trying to make combat for orbiter. There's a weapon class, containing the Fired vector, declared like this:

vector<VESSEL*> Fired;    //vector of all fired weapons of this class

When the fire() function is called, the vessel that is created is pushed t the back of fired, like this:

  OBJHANDLE objvessel=oapiCreateVesselEx (name.c_str(),classname.c_str(), &stat);

    Fired.push_back(oapiGetVesselInterface(objvessel));

(name,classname and stat are members of the vessel class)

The fire function works fine - weapons spawn perfectly. But when I simulate ground explosions, Orbiter just crashes. The debugger gives me this:
Unhandled exception at 0x00414de4 in orbiter.exe: 0xC0000005: Access violation reading location 0x00000000.

00414DE4 cmp dword ptr [eax+ebx*4],0

here is the code for ground explosions.

//We call all collision functions every timestep.
DLLCLBK void opcPreStep(double simt, double simdt, double mjd)
{
//Iterate through all fired weapons.
    for(vector<Weapon>::iterator it=WeaponClassList.begin();it<WeaponClassList.end();++it) {
        for(unsigned int i=0;i<it->Fired.size();i++) {
            //Ground explosions.
            if(it->Fired[i]->GetAltitude()<2.0) {
it->explode(i);
            }
        }
    }
}

Does anyone know what could be going wrong?

 

[DarkWanderer]

Try to:
1.Replace ++it with it++ (unlikely, but some compilers suffer from it)
2.Output the contents of "Fired" vector for each class e.g. on the vessel HUD.
After that the problem should be more clear.

 

[escapetomsfate]

Try to:
1.Replace ++it with it++ (unlikely, but some compilers suffer from it)

Nailed it! thank you.

Now I just have to figure out how to make vessel - weapon explosions.

EDIT: This is weird, some times I get the error and sometimes I don't :S

 

[dbeachy1]

EDIT: This is weird, some times I get the error and sometimes I don't :S

TBH I have never heard of a problem with 'it++' vs '++it', at least with MSVC++ compilers; the only difference between those two statements is that the former increments the iterator before the rest of the statement executes and the latter increments the iterator after the rest of the statement executes. Since the statement in this case is a single instruction (the last statement in the 'for' loop), it's moot. If it would make a difference in this context it would be a gruesome (and ridiculous) compiler bug.

Also, dumping the contents of a variable out on the HUD (or via oapiDebugString()) will not work in this case because the CTD occurs before the next frame is rendered, and so any pending writes to the screenbuffer will not actually appear on the screen before the CTD occurs.

What you really need to do is pin this down using the debugger: sooner or later you will need the debugger, and this is one of those times. By single-stepping through the code with the debugger you can see exactly which pointer is null instead of having to guess at it over and over. Using the debugger makes debugging large projects much easier.

Another thing you can do is to add 'assert' statements to your code to help detect intermittent bugs. For example:

ASSERT(MyPointer != NULL);

'ASSERT' statements are only generated for debug builds, so they won't slow down your release code at all. If you are running under the debugger and an assertion fails it will pop up the debugger right at the problem so you can look around. Even without the debugger, though, the assertion message will display the source file and line number where the problem occurred. You can find more information on asserts via Google. Here's a good place to start: http://www.codeguru.com/forum/showthread.php?t=315371

The next question is, are you 100% sure that the CTD is occurring inside the code you listed? Based on the assembly code that caused the CTD:

00414DE4 cmp dword ptr [eax+ebx*4],0

...that code is indexing an array in eax by ebx and comparing the pointer value to zero. In other words, the C++ source line is doing something like this:

if (SomeArray[SomeIndex][I] ==[/I] NULL)

...or comparing SomeArray[SomeIndex] to NULL (or '!= NULL) some other way (i.e., it is not necessarily inside an 'if' block). Without using the debugger or an assert that triggers, however, there is no way to even be sure which source line corresponds to the EIP in the assembly code listed with the crash. I don't see any code in the source block you posted that corresponds to the assembly code where the crash occurs, so that's why I suspect it's occurring somewhere else. In any case, the only way to track the problem down is to examine the source code where the actual CTD occurs and then go from there.

Back to asserts: here is an example of how you could use ASSERT to "sanity-check" the code in the block you listed (changes in red):

//We call all collision functions every timestep.
DLLCLBK void opcPreStep(double simt, double simdt, double mjd)
{
//Iterate through all fired weapons.
    for(vector<Weapon>::iterator it=WeaponClassList.begin();it<WeaponClassList.end();++it) {
        for(unsigned int i=0;i<it->Fired.size();i++) {
            //Ground explosions.
            [COLOR=red]VESSEL *pVessel = it->Fired[i][I][I];[/I][/I][/COLOR]
[COLOR=red]            ASSERT(pVessel != NULL);   // this check only exists in DEBUG builds[/COLOR]
            if(pVessel->GetAltitude()<2.0) {
                it->explode(i);
            }
        }
    }
}

The first thing I would do, however, is use the debugger to find out exactly which source code line is active when the CTD occurs.

 

[escapetomsfate]

Thanks for the help, dbeachy.

I've tried the debugger again, and now it says :

> CombatMFD.dll!std::_Iterator_base::~_Iterator_base() Line 162 + 0x10 bytes C++
CombatMFD.dll!std::vector<VESSEL *,std::allocator<VESSEL *> >:perator[](unsigned int _Pos=39194572) Line 783 + 0x8 bytes C++

 

[DarkWanderer]

TBH I have never heard of a problem with 'it++' vs '++it', at least with MSVC++ compilers;

There was at least one problem with for statement in MSVC2003, fixed in 2005: the variable declaration like

for (int i=0;...;...)

was considered by compiler to be outside the for cycle, and this trick is used in Orbiter code (at least in ShuttlePB SDK example source)
So there may be a problem with this too.

Also, dumping the contents of a variable out on the HUD (or via oapiDebugString()) will not work in this case because the CTD occurs before the next frame is rendered, and so any pending writes to the screenbuffer will not actually appear on the screen before the CTD occurs.

Most likely, CTD appears when the actions of the following code

            if(pVessel->GetAltitude()<2.0) {
                it->explode(i);

are performed. Of course, to debug it escapetomsfate should disable the troublesome code and look on vector contents first.

However, I see the point:

vector<VESSEL *,std::allocator<VESSEL *> >::operator[](unsigned int _Pos=39194572)

If you comment the string with explode(i), does error disappear? Looks like you've got an index error in this function.
And what is on line 162?

 

[dbeachy1]

There was at least one problem with for statement in MSVC2003, fixed in 2005: the variable declaration like

for (int i=0;...;...)

was considered by compiler to be outside the for cycle, and this trick is used in Orbiter code (at least in ShuttlePB SDK example source)
So there may be a problem with this too.

Yes, that bug is a well-known problem when porting MSVC++ 6.0 code to MSVC++ 7 or later; however, it has nothing to do with the code in question: that bug only involves leaving a variable in-scope after a for loop terminates, which is not related to overloaded pre- and post-increment operators on an object (in this case an iterator object) -- it's apples and oranges. That VC++ 6 bug certainly does not cause CTDs -- it runs just fine (the default DG sample code exhibits the bug, in fact). It only causes a compile-time error when porting code from VC6 to VC7 or newer -- I've dealt with it multiple times when porting VC6 code to VC7 or VC8. I don't mean to be argumentative here, but I don't want other developers to misunderstand that bug and go barking up the wrong tree worrying about "it++" vs. "++it" on an iterator in a for loop.

In any case, the group here can debate all day about what "might" be causing the problem, but the sure-fire way to pin it down is with the debugger -- that's the point I am trying to get across. I actually do speak from experience here. By using the debugger the problem could quite likely be pinned down and fixed in under ten minutes rather than spending hours trying different things. In addition, the danger in "trying different things until it works" is that the bug might not really be fixed (i.e., it could be changed and crash later in a different way). The key to fixing a bug is to thoroughly understand it before making any fix.

 

[escapetomsfate]

Yes, when I comment out it->explode(i), the error goes away. Explode looks like this:

void Weapon::explode(int idx) {
//Change the mesh.
Fired[idx]->InsertMesh(explodemesh.c_str(),0);
}

But that seems fine.

 

[Quick_Nick]

Can you try to see what it->Fired.size() returns after firing a couple of weapons?

 

[Hielor]

Presumably your explode() function deletes the object that has exploded. That's a problem, because the original vessel still has a pointer to it (in the vector). When you try to use that pointer, it may crash because that's no longer valid memory. It also may *not* crash because it hasn't yet freed that page, so the memory is technically still available (but it contains junk).

You could fix that by removing it from the vector when it explodes, but then you'd have to do iterator tricks in order to ensure that your iterator always remains valid (since it becomes invalid after you delete the element it refers to).

Your best bet would be to give your bullets or shells or whatever they are their own DLL, and have each one check for itself if it should explode.

 

[agentgonzo]

Presumably your explode() function deletes the object that has exploded.

It may be that Orbiter deletes the vessel object. From the SDK:

This function creates an interface to an existing vessel. It does not create a
new vessel. New vessels are created with the oapiCreateVessel and
oapiCreateVesselEx functions.

From this, it seems to me that orbiter may allocate and deallocate VESSEL classes at will - this is also indicated by the use of OBJHANDLEs rather than just relying on VESSEL pointers. Orbiter may delete the VESSEL class that you have a stored pointer to. You should store a vector of OBJHANDLEs rather than VESSEL pointers in your Fired vector.

Also, as dbeachy points out, run it through a debugger. VS has a debugger built in and as he's said (and from my experience also), using a debugger can save you (in some circumstances) literally *weeks* worth of effort.

 

[tblaxland]

How is WeaponClassList declared, initialised and populated? It appears that your intention is for this vector to contain a single instance of each type of Weapon, but I am interested to see how you are doing that.

Also, the Fired vector should be a static member of the weapon class, otherwise each instance of the weapon class will hold a separate Fired vector.

 

[Hielor]

It may be that Orbiter deletes the vessel object. From the SDK:

From this, it seems to me that orbiter may allocate and deallocate VESSEL classes at will - this is also indicated by the use of OBJHANDLEs rather than just relying on VESSEL pointers. Orbiter may delete the VESSEL class that you have a stored pointer to. You should store a vector of OBJHANDLEs rather than VESSEL pointers in your Fired vector.

Also, as dbeachy points out, run it through a debugger. VS has a debugger built in and as he's said (and from my experience also), using a debugger can save you (in some circumstances) literally *weeks* worth of effort.

He's calling oapiCreateVesselEx to create the vessel, and then storing a pointer to that vessel's interface. This would be a problem both in the case where Orbiter does something funky like what you say (which is doubtful), or in the case where that vessel has actually been deleted from the scenario (which I would bet is what's happening).

The best way to do what he's trying to do is to have each weapon object taking care of itself once fired, rather than being managed by the firing vessel. This is how I did it with my gatling gun, and those bullets happily deleted themselves after twenty seconds without the scenario crashing. IMO, that's a more efficient and logical method of doing things.

 

[tblaxland]

It may be that Orbiter deletes the vessel object. From the SDK:

From this, it seems to me that orbiter may allocate and deallocate VESSEL classes at will - this is also indicated by the use of OBJHANDLEs rather than just relying on VESSEL pointers. Orbiter may delete the VESSEL class that you have a stored pointer to. You should store a vector of OBJHANDLEs rather than VESSEL pointers in your Fired vector.

This has not been my experience. I did some pretty extensive testing in this regard for StateVectorMFD. That MFD caches VESSEL pointers with no unexpected problems. For vessels, the VESSEL class existed for as long as the OBJHANDLE was valid, ie, until the simulation session ended or the VESSEL was deleted. Also, the VESSEL class has no virtual destructor so if Orbiter was deleting these objects at will we would likely see heap corruption, as discussed in this thread, this post. The MFD class does not suffer from this problem due to its virtual destructor and Orbiter does delete MFD objects at will. That is the reason for the ovcExit function - so that vessel DLL can delete its own objects.

I expect that the exploding objects are deleted at some stage though and they should remove themselves from the Fired vector. The code for this could be put in the weapon's destructor. Also, I assume you are deriving objects from the Weapon class - make its destructor virtual so that destructor of the derived class gets called.

The OBJHANDLEs provide a convenient method for building a list of objects in the solar system like, for example, when you want a list of camera targets.

Also, as dbeachy points out, run it through a debugger. VS has a debugger built in and as he's said (and from my experience also), using a debugger can save you (in some circumstances) literally *weeks* worth of effort.

I also strongly recommend that.

 

[Hielor]

How is WeaponClassList declared, initialised and populated? It appears that your intention is for this vector to contain a single instance of each type of Weapon, but I am interested to see how you are doing that.

Also, the Fired vector should be a static member of the weapon class, otherwise each instance of the weapon class will hold a separate Fired vector.

It looks like the WeaponClassList is a list of the different types of weapons, each of which has a vector of all fired weapons of that type.

Just have each fired weapon blow itself up if it hits the ground. It's simpler, safer, and more extensible.

 

[tblaxland]

It looks like the WeaponClassList is a list of the different types of weapons, each of which has a vector of all fired weapons of that type.

In which case it must contain a single instance of each of those classes, and the Fired most definitely needs to be a static member.

Just have each fired weapon blow itself up if it hits the ground. It's simpler, safer, and more extensible.

I agree.

 

[dbeachy1]

One thing to remember regarding the use of cached OBJHANDLE and/or VESSEL * values: a vessel may be deleted at any time by 1) the scenario editor, 2) the vessel itself, or 3) by some other add-on. Once that happens any VESSEL pointers or OBJHANDLE values that reference the target vessel become invalid. If an add-on then tries to access the deleted vessel via a cached VESSEL pointer or by using a VESSEL pointer retrieved by passing the cached OBJHANDLE to oapiGetVesselInterface, a CTD will occur. Add-ons that cache vessel handles should test the handle once each frame before it is used by checking the handle with oapiIsVessel first (you could also check whether oapiGetVesselInterface returns NULL before using the pointer). If oapiIsVessel returns false, the cached vessel handle should be discarded since the vessel no longer exists. It is better to cache the OBJHANDLE rather than the VESSEL pointer itself since it is easier to check whether the target vessel has been deleted. From the oapiIsVessel API docs:

oapiIsVessel
...

Notes:​

· This function can be used to test if a previously obtained vessel handle is still valid. A handle becomes invalid if the associated vessel is deleted.

· An alternative to using oapiIsVessel is monitoring vessel deletions by implementing the opcDeleteVessel callback function in the plugin module.

In any case, regardless of how the add-on does it, it needs to be careful to verify once per frame that any cached vessel handle or pointer is still valid before using the VESSEL pointer again. Not handling this condition is a relatively common error, which is why certain add-ons do not work with the Orbiter scenario editor and the XR payload dialog (i.e., where vessels are dynamically created and deleted). Just something to watch out for.

 

[tblaxland]

There is also the undocumented opcDeleteVessel function:

DLLCLBK void opcDeleteVessel (OBJHANDLE hVessel)

Are opcXXX class functions detected in vessel modules? I would expect that vessel modules are loaded differently to plugin modules and Orbiter may not look for that class of function when it loads them.

 

[escapetomsfate]

Presumably your explode() function deletes the object that has exploded. That's a problem, because the original vessel still has a pointer to it (in the vector). When you try to use that pointer, it may crash because that's no longer valid memory. It also may *not* crash because it hasn't yet freed that page, so the memory is technically still available (but it contains junk).

Nope. It just replaces the mesh with an explosion mesh.

How is WeaponClassList declared, initialised and populated? It appears that your intention is for this vector to contain a single instance of each type of Weapon, but I am interested to see how you are doing that.

Also, the Fired vector should be a static member of the weapon class, otherwise each instance of the weapon class will hold a separate Fired vector.

Weapons are added to WeaponClassList with a manual AddToVec function. I might replace WeaponClassList with members in a seperate CombatStat class, rather than a gloobal one.

Also, I want a seperate Fired Vector, so that I can keep track of each fired weapon by it's class.

It looks like the WeaponClassList is a list of the different types of weapons, each of which has a vector of all fired weapons of that type.

You're right, WeaponClassList is just a list of all created Weapon Classes.

Just have each fired weapon blow itself up if it hits the ground. It's simpler, safer, and more extensible.

To be honest, I'm trying to make a sort of framework for combat, so all the hard stuff is already done for a developer.

The source code will be released on my site soon, just download the link in my sig.

 

[Hielor]

Nope. It just replaces the mesh with an explosion mesh.

So the bullets/shells/whatever never actually get destroyed? That's bad. The more you fire, the more vessels get added to the scenario and the more Orbiter will slow down until it becomes unusable. You have to delete the things sometime.

To be honest, I'm trying to make a sort of framework for combat, so all the hard stuff is already done for a developer.

And if your code crashes whenever a bullet/shell/whatever hits the ground, or if it fills the scenario with old and undeleted bullet objects, no one will use your framework.