Torque 3D's Achilles heel
by Konrad Kiss · in Torque 3D Professional · 03/24/2012 (11:49 am) · 184 replies
Recently I spent some time getting myself familiar with static code analysis and the more common (indie-budget freindly) tools associated with the topic. #AltDevBlogADay had several posts about such tools by Bruce Dawson and John Carmack. Definitely worth checking out.
So, it all began with me having a very elusive bug somewhere in code. I hoped that it would be something that a static code analyzer would pick up, so I lined up what I could from trial versions to open source software, and got some amazing results once these went through the Torque 3D 1.2 codebase.
I can't remember if the actual problem was found by these tools (it's been a while ago). What I recall is being taken aback by the results I got. But first a disclaimer...
I've been reviewing engine code changes for the past 4 years. Torque 3D is huge. One of the most surprising thing was the fact, that these code analyzers could - proportionally - find only a few problems. However, the few they did find were hard to see even when I knew the problem.
The kinds of problems I found primarily were:
- Uninitialized member variables used without initialization
- Checking the existence of an object and using the same object outside the check condition
- Passing local pointers that could become invalid
- Using && instead of &, || instead of | operations
- Bad casts for variables while the code assumes another cast (ie. U32 vs S32 - especially when negative values are used for error codes - common in some of the linked libraries)
- Typos, cut & paste programming mistakes that compile (probably the most vicious of all)
I'm only going to list my favorites, hoping that this will be a big enough motivation for everyone to start using some basic form of static code analysis. The complete list would be way too long for me to list here.
gfxD3D9TextureManager: using boolean evaluation against variables that were clearly intended to be bitmasks.
guiIconButtonCtrl.cpp: find the highlighted button, I dare you
processedFFMaterial.cpp: making sure (though what about blendDest?)
mRectF.cpp: intersectTriangle
winDInputDevice.cpp: POV_down, not much of a choice (twice in the same file)
rectClipper: clipRect (near eof)
win32Window.cpp: equilibrium
Finally - I need to clarify - by all means, no disrespect was intended towards Torque 3D developers. I would have done far worse most likely. I can't count the many new things in the source that made me go "Wow" and "Yey" - it is clearly visible that the team is bigger and better at what they do. Torque 1.2 is the best Torque yet - and I mean it by comparing the overall usability, the number of bugs, etc..
With this post I'm only trying to point at a solution that can save everyone from such blatant bugs in the future, while - perhaps - also nudging GG a little to take a step in the right direction. ;)
Cheers
--Konrad
So, it all began with me having a very elusive bug somewhere in code. I hoped that it would be something that a static code analyzer would pick up, so I lined up what I could from trial versions to open source software, and got some amazing results once these went through the Torque 3D 1.2 codebase.
I can't remember if the actual problem was found by these tools (it's been a while ago). What I recall is being taken aback by the results I got. But first a disclaimer...
I've been reviewing engine code changes for the past 4 years. Torque 3D is huge. One of the most surprising thing was the fact, that these code analyzers could - proportionally - find only a few problems. However, the few they did find were hard to see even when I knew the problem.
The kinds of problems I found primarily were:
- Uninitialized member variables used without initialization
- Checking the existence of an object and using the same object outside the check condition
- Passing local pointers that could become invalid
- Using && instead of &, || instead of | operations
- Bad casts for variables while the code assumes another cast (ie. U32 vs S32 - especially when negative values are used for error codes - common in some of the linked libraries)
- Typos, cut & paste programming mistakes that compile (probably the most vicious of all)
I'm only going to list my favorites, hoping that this will be a big enough motivation for everyone to start using some basic form of static code analysis. The complete list would be way too long for me to list here.
gfxD3D9TextureManager: using boolean evaluation against variables that were clearly intended to be bitmasks.
if (retTex->mProfile->isRenderTarget() && mslevel != 0 && (mDeviceCaps.Caps2 && D3DDEVCAPS2_CAN_STRETCHRECT_FROM_TEXTURES))
guiIconButtonCtrl.cpp: find the highlighted button, I dare you
ColorI fontColor = mActive ? (highlight ? mProfile->mFontColor : mProfile->mFontColor) : mProfile->mFontColorNA;
processedFFMaterial.cpp: making sure (though what about blendDest?)
result.blendSrc = GFXBlendOne;
result.blendSrc = GFXBlendOne;mRectF.cpp: intersectTriangle
if(contains(a) || contains(b) || contains(b))
winDInputDevice.cpp: POV_down, not much of a choice (twice in the same file)
newEvent.objInst = ( objInst == 0 ) ? SI_DPOV : SI_DPOV
rectClipper: clipRect (near eof)
out_rRect.extent.x = bottomR.x - out_rRect.point.x + 1; out_rRect.extent.x = bottomR.y - out_rRect.point.y + 1;
win32Window.cpp: equilibrium
newLeft = mClamp(newLeft, 0, newLeft);
newTop = mClamp(newLeft, 0, newTop);Finally - I need to clarify - by all means, no disrespect was intended towards Torque 3D developers. I would have done far worse most likely. I can't count the many new things in the source that made me go "Wow" and "Yey" - it is clearly visible that the team is bigger and better at what they do. Torque 1.2 is the best Torque yet - and I mean it by comparing the overall usability, the number of bugs, etc..
With this post I'm only trying to point at a solution that can save everyone from such blatant bugs in the future, while - perhaps - also nudging GG a little to take a step in the right direction. ;)
Cheers
--Konrad
About the author
http://about.me/konrad.kiss
#142
05/29/2012 (12:16 pm)
Thanks Scott and Alfio!
#145
06/03/2012 (4:04 pm)
Hey Alfio, can I play?: Thanks!
#146
06/05/2012 (12:59 pm)
Give me your email please :)
#148
06/06/2012 (10:32 am)
Getting back to the improvements discussion, I was wondering if anyone else is interested in 32-bit index buffer support? I am in need of it for a project I'm doing and wanted to see what people think about the best way to implement it. Thoughts?
#149
06/06/2012 (10:44 am)
To which index buffers you mean?
#150
I had a non-game project with Torque 3D once where I had to use really large models, and they wouldn't load because the index buffer would be too small. I don't think this happens too often, but it would allow us to save and load an entire mission's static files as a single model which could be a nice feature for some games where everything is both in scope and on screen anyway, as this would make artists' life much easier I imagine.
I was under the impression that the DTS format itself supported 32 bit indices and that the bottleneck was somewhere else, but I really don't have a clue. :)
06/06/2012 (10:46 am)
Does DTS support that many indices? Or is it the format itself that you extend?I had a non-game project with Torque 3D once where I had to use really large models, and they wouldn't load because the index buffer would be too small. I don't think this happens too often, but it would allow us to save and load an entire mission's static files as a single model which could be a nice feature for some games where everything is both in scope and on screen anyway, as this would make artists' life much easier I imagine.
I was under the impression that the DTS format itself supported 32 bit indices and that the bottleneck was somewhere else, but I really don't have a clue. :)
#151
06/06/2012 (10:49 am)
I think he means index buffers used against vertex buffers for models.
#152
06/06/2012 (10:52 am)
Also the DirectX level support the 32bit model for the indexBuffer.
#153
06/06/2012 (11:01 am)
I can try to apply a patch to the CE to enable the 32bit indexbuffer and try if work correctly.
#154
Implementing it doesn't seem too hard (at least for the manual case, rather than for meshes loaded from DTS) but the interface seems a little messy if you just do something simple like adding an optional parameter.
Right now it just always assumes you always want a 16-bit buffer.
06/06/2012 (11:04 am)
I'm not sure if DTS supports it or not. I didn't think of it because at least for my case, the mesh is built specifically for the object and doesn't use DTS. Obviously though that would be great to have support for it through DTS as well.Implementing it doesn't seem too hard (at least for the manual case, rather than for meshes loaded from DTS) but the interface seems a little messy if you just do something simple like adding an optional parameter.
Right now it just always assumes you always want a 16-bit buffer.
#155
The dts exposes the indexes a 32 bits
The size of my indexBuffer have the 32bit size:
But the result is this:

Should i make debug for matching the indices are imported from the dts with those passed to the primitives rendering.
06/06/2012 (4:14 pm)
Apparently i forgot to change something.The dts exposes the indexes a 32 bits
if (TSShape::smReadVersion > 25)
{
// mesh primitives (start, numElements) and indices are stored as 32 bit values
szPrimIn = tsalloc.get32();
primIn = (TSDrawPrimitive*)tsalloc.getPointer32(szPrimIn*3);
szIndIn = tsalloc.get32();
indIn = tsalloc.getPointer32(szIndIn);
}The size of my indexBuffer have the 32bit size:
mD3DDevice->CreateIndexBuffer( sizeof(U32) * numIndices , usage, GFXD3D9IndexFormat[GFXIndexFormat32], pool, &res->ib, 0)
But the result is this:

Should i make debug for matching the indices are imported from the dts with those passed to the primitives rendering.
#156
06/06/2012 (4:19 pm)
Are you sure the dts exporter supports the 32 bit depth? I have faint memories of such issues with some exporters.
#157
I can apply this change to the CE, but disabled.
06/06/2012 (4:32 pm)
No Konrad, i should debug all to check the right size and indices start/len.I can apply this change to the CE, but disabled.
#158
06/06/2012 (4:41 pm)
Interesting find Alfio. I'd be interested to hear whether or not the exporters handle it properly or not.
#159
Rene and Tom give a pretty good explanation on how it works and why. I believe that many exporters also default to capping index buffer bit size at 16 bits, so even if you changed the engine, it would not work without rewriting some of the (most?) exporters. All in all, the fact that some cards still don't support a 32 bit depth for the index buffer sounds pretty scary. I don't think we should change it in the CE.
06/06/2012 (4:56 pm)
Hey guys. This thread is worth checking out: http://www.garagegames.com/community/forums/viewthread/94955Rene and Tom give a pretty good explanation on how it works and why. I believe that many exporters also default to capping index buffer bit size at 16 bits, so even if you changed the engine, it would not work without rewriting some of the (most?) exporters. All in all, the fact that some cards still don't support a 32 bit depth for the index buffer sounds pretty scary. I don't think we should change it in the CE.
#160
Also, I see 32bit indices being more useful for custom objects than necessarily for DTSes (which as you mentioned would need exporters to be fixed as well).
That said I do think 32bit indices will be or are already more common for sure in commercial engines than they were in 2009.
06/06/2012 (5:31 pm)
@Konrad, I don't think GPU support is an issue at this point. Keep in mind those posts are from 2009. Also, I see 32bit indices being more useful for custom objects than necessarily for DTSes (which as you mentioned would need exporters to be fixed as well).
That said I do think 32bit indices will be or are already more common for sure in commercial engines than they were in 2009.
Associate Scott Burns
GG Alumni
You can go ahead and add Ross, as an Associate he has access to 1.2 and more already anyway.