Game Development Community

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.
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
NextNext Thread
PrevPrevious Thread
More from Torque 3D Professional
Page«First 1 2 3 4 5 6 7 Last »

Torque Owner Vince Gee
WinterLeaf Entertainment
#21
04/05/2012 (6:53 pm)
Alfio,

You think there is any way I could get a copy of the svn diff?

Vince

Torque Owner Demolishun
DemolishunConsulting Rocks!
#22
04/06/2012 (1:38 am)
@Alfio,
That is excellent. Good job!

@GG,
After reading through this thread and thinking about it I would vote that the next release 1.3 (or whatever) focus on this issue above all others. I would definitely pay an upgrade price $20 to $30 or so for a good code analysis and correction of the entire engine. The engine is very feature full in my opinion, but having the stability checked and made more robust would definitely save everyone time down the road. I am really appreciative of the other GG members that have contributed toward this and I hope these kinds of fixes get put into the engine.

Torque Owner Alfio Saitta
Collateral Studios
#23
04/06/2012 (7:28 am)
@Vince
Yes, of course. But first, i want to fix a few more calculation issue that takes many cpu cycles. For example, replace some multiplication (IMUL is very heavy for the processor), and eliminate some data compare if possible.

Torque Owner elvince
Neo Studio
#24
04/06/2012 (11:45 am)
That s an excellent topic. As proposed, If some svn diff can be shared that will profit to the full community.
I m looking forward to it.

@alfio,
30% seems huge. I hope GG will work with you to get this changes implemented as core source in a next short time patch.

Good to see that people are keen to optimize the engine and shared hours of work to the community.

Thanks!!!

Torque Owner Vince Gee
WinterLeaf Entertainment
#25
04/10/2012 (4:59 am)
Alfio,

If you get a chance can you shoot me an email at vgee<at>winterleafentertainment.com

I'd like to hear more about what you did.

Vince

Torque Owner Alfio Saitta
Collateral Studios
#26
04/10/2012 (5:43 am)
I finished a first analysis and have removed more than 60 code problems. Now I switched to simplified by removing the call of the same type inside the cycles or comparisons. In fact there are many other cases where one function is called inside of a single cycle. For example the function

image.shapeInstance[i]->getShape()

are called 10 times in the same loop.

Torque Owner Alfio Saitta
Collateral Studios
#27
04/10/2012 (6:47 am) 
void ShapeBase::updateAnimThread(....)
{
   ....
   for (U32 i=0; i<ShapeBaseImageData::MaxShapes; ++i)
   {
      ....
#ifdef SA_OPTIMIZATIONS_STAGE4
      TSShape *mShape = image.shapeInstance[i]->getShape();
#endif
      if (image.animThread[i] && stateData.sequence[i] != -1) 
      {
         ....
         if (hasShapeBasePrefix || hasScriptPrefix)
         {
            ....
#ifndef SA_OPTIMIZATIONS_STAGE4
            String baseSeqName(image.shapeInstance[i]->getShape()->getSequenceName(stateData.sequence[i]));
#else
            String baseSeqName(mShape->getSequenceName(stateData.sequence[i]));
#endif
            if (!found && hasShapeBasePrefix && hasScriptPrefix)
            {
               ....
#ifndef SA_OPTIMIZATIONS_STAGE4
               S32 index = image.shapeInstance[i]->getShape()->findSequence(seqName);
#else
               S32 index = mShape->findSequence(seqName);
#endif
               ....
            }

            if (!found && hasShapeBasePrefix)
            {
               ....
#ifndef SA_OPTIMIZATIONS_STAGE4
               S32 index = image.shapeInstance[i]->getShape()->findSequence(seqName);
#else
               S32 index = mShape->findSequence(seqName);
#endif
               ....
            }

            if (!found && hasScriptPrefix)
            {
               ....
#ifndef SA_OPTIMIZATIONS_STAGE4
               S32 index = image.shapeInstance[i]->getShape()->findSequence(seqName);
#else
               S32 index = mShape->findSequence(seqName);
#endif
               ....
            }
         }

         if (seqIndex != -1)
         {
            ....

            if (stateData.flashSequence[i] == false) 
            {
               ....

               // Broadcast the sequence change
#ifndef SA_OPTIMIZATIONS_STAGE4
               String seqName = image.shapeInstance[i]->getShape()->getSequenceName(stateData.sequence[i]);
#else
               String seqName = mShape->getSequenceName(stateData.sequence[i]);
#endif
               ....
            }
            else
            {
               ....

               // Go through the same process as the animThread sequence to find the flashThread sequence
               if (hasShapeBasePrefix || hasScriptPrefix)
               {
                  ....
#ifndef SA_OPTIMIZATIONS_STAGE4
                  String baseVisSeqName(image.shapeInstance[i]->getShape()->getSequenceName(stateData.sequenceVis[i]));
#else
                  String baseVisSeqName(mShape->getSequenceName(stateData.sequenceVis[i]));
#endif

                  if (!found && hasShapeBasePrefix && hasScriptPrefix)
                  {
                     ....
#ifndef SA_OPTIMIZATIONS_STAGE4
                     S32 index = image.shapeInstance[i]->getShape()->findSequence(seqName);
#else
                     S32 index = mShape->findSequence(seqName);
#endif

                     if (index != -1)
                     {
                        ....
                     }
                  }

                  if (!found && hasShapeBasePrefix)
                  {
                     ....
#ifndef SA_OPTIMIZATIONS_STAGE4
                     S32 index = image.shapeInstance[i]->getShape()->findSequence(seqName);
#else
                     S32 index = mShape->findSequence(seqName);
#endif
                     ....
                  }

                  if (!found && hasScriptPrefix)
                  {
                     ....
#ifndef SA_OPTIMIZATIONS_STAGE4
                     S32 index = image.shapeInstance[i]->getShape()->findSequence(seqName);
#else
                     S32 index = mShape->findSequence(seqName);
#endif
                     ....
                  }
               }

               ....

               // Broadcast the sequence change
#ifndef SA_OPTIMIZATIONS_STAGE4
               String seqName = image.shapeInstance[i]->getShape()->getSequenceName(stateData.sequenceVis[i]);
#else
               String seqName = mShape->getSequenceName(stateData.sequenceVis[i]);
#endif
               ....
            }
         }
      }
   }
}

Torque Owner Vince Gee
WinterLeaf Entertainment
#28
04/19/2012 (5:41 am)
Any updates on this alfio?

Torque Owner Alfio Saitta
Collateral Studios
#29
04/19/2012 (6:29 am)
Until now applied more than 90 changes, but still there are many to replace. Honestly, i loosened a bit this work because am working on some shaders and other projects with other engines.

Torque Owner Demolishun
DemolishunConsulting Rocks!
#30
04/19/2012 (7:52 pm)
@Alfio,
I just noticed your link to the AMD profiler. I am definitely going to use that as part of my interface testing for my Python extension. I am interested in seeing what the real cost of using autogenerated C++ interfaces by SWIG is. Also, it would be interesting to see how my own interfaces and custom Python functions stack up. I don't know why, but I really love to see the assembler behind it all. Sick and twisted I know.

Thanks for the link.

Torque 3D Owner Morrock
#31
04/25/2012 (11:56 pm)
I would love to see the diff of this work, even if it's not done. 30% seems like a huge improvement, and optimizing T3D is something I've always thought was necessary, as I don't run on the best hardware (and a lot of gamers don't either).

Torque Owner Alfio Saitta
Collateral Studios
#32
04/28/2012 (12:38 pm)
Sorry everyone if i do not answer ever suffered, but the work does not allow it.

Unfortunately now going to release an early version of the diff, would mean going to releasing a further version, and have multiple copies of the same engine. So it is better that before i finish the job, and then releasing the final diff.

I think often these days that a shared copy on an svn server would be the ideal. But it would take some work for the maintenance and management, and especially the blessing of the GG.

Torque Owner Vince Gee
WinterLeaf Entertainment
#33
05/02/2012 (10:43 am)
I have a SVN server which I could dedicate to the cause, the only thing I would need to make sure of is that GG would be ok with it. I have no problem limiting access.

I also have a TFS server as well, I am willing to offer either to this project. My bandwidth is pretty decent as well.

Vince

Torque Owner Demolishun
DemolishunConsulting Rocks!
#34
05/02/2012 (5:51 pm)
If the server is like XP-Dev you can give access to just those that confirm to have T3D licenses. Also, limit it to diffs if possible. Then even if someone compromises the archive they don't actually get anything useful. I have access to XP-Dev and they have https access for SVN repos. I assume you are using something similar.

Associate David Montgomery-Blake
David MontgomeryBlake
#35
05/11/2012 (1:07 pm)
As long as you ensure that the repo is private and only available to licensees, you should be able to provide changes.

Michael Tornberg
PlanetRift
#36
05/12/2012 (10:17 am)
I think it would be much better if those changes were put here at GG on a private svn (or similar) hosted by GG. Then GG could also review changes (and if they are good) they could find their way to the official code trunk.

Just a suggestion, because then the community could speed things up on the development of T3D. Just let GG steer things, much like Linus Torvalds for Linux.


Torque 3D Owner Paul Yoskowitz
WinterLeaf Entertainment
#37
05/12/2012 (1:19 pm)
@Michael - I see a ton of arguments each way. At the end of the day, its their code base, their decision. Im good either way really.

Associate Michael Hall
Distracted...
#38
05/12/2012 (1:44 pm)
Making an educated guess here, but I don't *think* GG would have the bandwidth or the resources to invest in an 'official' community repository.

We could certainly have a community initiative that would work much like the CEV does for TorqueX. Someone will need to be an administrator and check for licenses before allowing access granting read/write privileges on an individual basis. Fortunately licenses are easily checked by looking at someone's profile or badge here on the forums. Maintenance and subscription fees could come from community donations. Someone trustworthy and reliable would have to head such an initiative.

Associate Konrad Kiss
Bitgap Games
#39
05/12/2012 (2:14 pm)
One way to do it could be creating a private repository on github to not have to worry about maintenance, software updates, bandwidth and security. We could start a thread in a private forum like this one where access would be requested. As for who should head the initiative - I think it should at all times be two active and reliable community members preferably from two distinct time zones. Optionally, GG would have them sign an agreement for this to lower the chance of misuse. I think for this we would need the Bronze business plan that costs $25 a month. Perhaps even GG could help us with paying for some of that.

Michael Tornberg
PlanetRift
#40
05/13/2012 (1:24 am)
Hmmm. I don't think using the GG lawyers would be ok with github. Because then people running github would essentially have free access to the source (from a legal standpoint, not really saying they are thieves :) But the risk cannot be ignored regardless, it only takes one bad apple..) and there is also the hackers to consider. All risks added, I don't think GG lawyers would be OK with that.

Page«First 1 2 3 4 5 6 7 Last »