Bugs and errata

    This site uses cookies. By continuing to browse this site, you are agreeing to our Cookie Policy.

    • Here we go!

      Source Code

      1. class HumanView : public IGameView
      2. {
      3. ...
      4. int m_PointerRadius;
      5. ...


      the variable is assigned in the constructor but it's never used in the HumanView::VOnMsgProc() func, instead the value of hardcoded 1 is passed to the input handlers, e.g.

      Source Code

      1. result = m_PointerHandler->VOnPointerMove(Point(LOWORD(msg.m_lParam), HIWORD(msg.m_lParam)), 1);
      Looking for a job!
      My LinkedIn Profile

      The post was edited 1 time, last by devast3d ().

    • Source Code

      1. void BaseGameLogic::VAddView(shared_ptr<IGameView> pView, ActorId actorId)
      2. {
      3. // This makes sure that all views have a non-zero view id.
      4. int viewId = static_cast<int>(m_gameViews.size());
      5. m_gameViews.push_back(pView);
      6. pView->VOnAttach(viewId, actorId);
      7. pView->VOnRestore();
      8. }


      First call to this function will result in zero viewId that is contrary to the comment. Probably first two lines should be swapped?
      Looking for a job!
      My LinkedIn Profile

      The post was edited 1 time, last by devast3d ().

    • You know - for the life of me i can't recall why a non-zero view id is important. I think the comment should just be nuked from the code.
      Mr.Mike
      Author, Programmer, Brewer, Patriot

    • Here are several typos I found while reading.

      • page 286. There is class definition CMessageBox however further the class is named MessageBox
      • page 316. virtual bool VTriggerVTriggerEvent(const IEventDataPtr& pEvent) const; Seems like just VTriggerEvent
      • page 317. virtual bool VTickVUpdate() here and throughout the events chapter
      • page 325. GameResumedResumed The game is resumed.
      Looking for a job!
      My LinkedIn Profile

    • Virtual Destructors

      First of all, thanks for writing a really interesting book. I've learned a lot.

      That being said, I do have a request for a fix for future editions. Pages 835-836 describe problems with leaking memory due to the lack of the "virtual" keyword on the destructor of a derived class (LeakyClass). You point out that delete'ing a pointer to the base class will not run the derived class's destructor.

      However, we cannot know from the code provided whether that destructor would run or not. For this scenario to work correctly, it is required that SomeBaseClass's destructor be marked as virtual, as it is the compile-time type of the pointer being deleted. If you left ~SomeBaseClass as non-virtual and marked ~LeakyClass as virtual, you would still leak the memory.

      On a side note, not only does marking ~SomeBaseClass as virtual solve your problem, it also implicitly makes any derived class's destructor virtual as well, whether the derived class marks their destructor as virtual or not. The same holds true for regular class members. As soon as a base class declares a method to be virtual, all matching methods in derived classes are also virtual whether they are declared so or not.

    • That looks like it was 'fixed' by an editor to me - it should read 'Allbricks.bmp' and 'AllBricks.bmp'.
      Mr.Mike
      Author, Programmer, Brewer, Patriot

    • I found some strange code in the ResCache.cpp file of the source code.

      Source Code

      1. // ResCache.cpp
      2. int ResourceZipFile::VGetRawResource(const Resource &r, char *buffer)
      3. {
      4. int size = 0;
      5. optional<int> resourceNum = m_pZipFile->Find(r.m_name.c_str());
      6. if (resourceNum.valid())
      7. {
      8. size = m_pZipFile->GetFileLen(*resourceNum);
      9. m_pZipFile->ReadFile(*resourceNum, buffer);
      10. }
      11. return size;
      12. }
      Display All

      m_pZipFile->ReadFile() returns bool value indicating whether the read operation was successful however it's ignored by the VGetRawResource() method, so we could end up with the bad data in the buffer.

      -------------------------------
      And another one.

      Source Code

      1. shared_ptr<ResHandle> ResCache::Load(Resource *r)
      2. {
      3. ...
      4. int allocSize = rawSize + ((loader->VAddNullZero()) ? (1) : (0));
      5. char *rawBuffer = loader->VUseRawFile() ? Allocate(allocSize) : GCC_NEW char[allocSize];
      6. memset(rawBuffer, 0, allocSize);
      8. if (rawBuffer==NULL || m_file->VGetRawResource(*r, rawBuffer)==0)
      9. {
      10. // resource cache out of memory
      11. return shared_ptr<ResHandle>();
      12. }
      13. ...
      Display All

      First thing is there is the chance to memset() invalid rawBuffer (as the check is done in the next line). I don't really know what will happen if we do so.

      Second, if rawBuffer is not NULL but VGetRawResource() equals to 0 then we have two options:
      1. rawBuffer was allocated inside Allocate()
      2. rawBuffer was allocated here in the function GCC_NEW char[allocSize]

      If we just return from the function in the first case memory will be allocated forever and the cache will think it has some resource (due to Allocate() adds allocSize to the m_allocated). In the second case it's a no-brain memory leak.

      Next, follow up of the very same function (read notes inside the code)

      Brainfuck Source Code

      1. ...
      2. char *buffer = NULL;
      3. unsigned int size = 0;
      5. if (loader->VUseRawFile())
      6. {
      7. buffer = rawBuffer;
      8. handle = shared_ptr<ResHandle>(GCC_NEW ResHandle(*r, buffer, rawSize, this));
      9. }
      10. else
      11. {
      12. size = loader->VGetLoadedResourceSize(rawBuffer, rawSize);
      13. buffer = Allocate(size);
      14. if (rawBuffer==NULL || buffer==NULL)
      15. {
      16. // ------------------------------------
      17. // Another memory leak in the case when rawBuffer != NULL
      18. // but buffer == NULL. rawBuffer will leak.
      19. // ------------------------------------
      20. return shared_ptr<ResHandle>();
      21. }
      23. handle = shared_ptr<ResHandle>(GCC_NEW ResHandle(*r, buffer, size, this));
      24. bool success = loader->VLoadResource(rawBuffer, rawSize, handle);
      26. // [mrmike] - This was added after the chapter went to copy edit. It is used for those
      27. // resoruces that are converted to a useable format upon load, such as a compressed
      28. // file. If the raw buffer from the resource file isn't needed, it shouldn't take up
      29. // any additional memory, so we release it.
      30. if (loader->VDiscardRawBufferAfterLoad())
      31. {
      32. // ------------------------------------
      33. // What's happened to the rawBuffer when this if is not hit? leak again?
      34. // ------------------------------------
      35. SAFE_DELETE_ARRAY(rawBuffer);
      36. }
      38. if (!success)
      39. {
      40. // resource cache out of memory <---------- false comment?
      41. return shared_ptr<ResHandle>();
      42. }
      43. }
      45. if (handle)
      46. {
      47. m_lru.push_front(handle);
      48. m_resources[r->m_name] = handle;
      49. }
      51. GCC_ASSERT(loader && _T("Default resource loader not found!"));
      52. return handle; // ResCache is out of memory! <---------- false comment?
      53. } // End of the Load
      Display All


      ------------------
      Edit: Yet another bug in the Load func. lol

      Source Code

      1. buffer = rawBuffer;
      2. handle = shared_ptr<ResHandle>(GCC_NEW ResHandle(*r, buffer, rawSize, this));

      ResHandle constructor is initialized with wrong buffer size variable. Instead of rawSize it should be allocSize otherwise we will miss 1 byte that could be added by VAddNullZero()
      Looking for a job!
      My LinkedIn Profile

      The post was edited 6 times, last by devast3d ().

    • Follow up question.

      In the end of the Load() method there is check of the handle variable

      Source Code

      1. if (handle)
      2. {
      3. m_lru.push_front(handle);
      4. m_resources[r->m_name] = handle;
      5. }

      handle is of shared_ptr<ResHandle> type. So basically we check if allocation of GCC_NEW ResHandle succeded. But what will happen if the allocation fails?

      AFAIK some std exception will be thrown, which then makes all the allocation checks useless. There is the option in VS to disable exceptions however TeapotWars solution enables them.

      Source Code

      1. Enable C++ Exceptions: Yes (/EHsc)

      What's more interesting, what will happen if we disable exceptions and try to allocate too much memory via new operator? The object constructor probably won't run, I presume destructor won't run either.

      Back to our case. If we disable exceptions and have our handle==NULL then we must yet again free buffers manually or they will leack.
      If exceptions are enabled seems like the app will just die.

      Am I right?
      Looking for a job!
      My LinkedIn Profile

    • Why would an exception be thrown? shared_ptr has its evaluation operator overloaded to evaluate the validity of the internal pointer, if it fails, it simply won't be added to the resource cache, and will fall out of scope and have it's memory released, I wasn't entirely sure what you were trying to figure out here, but if you meant how do you handle failed resources on the game logic side, I personally think assertions are the right choice, I suppose you could put it into the resource loading code itself, I have found myself doing it for each resource at the point where I load it.

      *Edit*
      I read it twice, you mean what would happen if the actual memory allocation failed? I assume that with assertions on it will give the typical assertion from the OS, but will the code progress after this or does it fail and go into a stack unwind? It would be nice if the allocation failed, null was returned and you could check this, and give your own nice pretty message box for an end user to see, but I doubt it you more than likely get some weird 0xCDCDCDCD
      PC - Custom Built
      CPU: 3rd Gen. Intel i7 3770 3.4Ghz
      GPU: ATI Radeon HD 7959 3GB
      RAM: 16GB

      Laptop - Alienware M17x
      CPU: 3rd Gen. Intel i7 - Ivy Bridge
      GPU: NVIDIA GeForce GTX 660M - 2GB GDDR5
      RAM: 8GB Dual Channel DDR3 @ 1600mhz

      The post was edited 1 time, last by mholley519 ().

    • Yes, I was talking about actual memory alloc.

      I found that operator new is not trivial at all.
      According to the wikipedia and some stackoverflow threads, many things can happen.
      For example in stackoverflow.com/questions/94…-deal-with-bad-alloc-in-c

      An allocation function that fails to allocate storage can invoke the currently installed new_handler(18.4.2.2), if any. [Note: A program-supplied allocation function can obtain the address of the currently installed new_handler using the set_new_handler function (18.4.2.3).] If an allocation function declared with an empty exception-specification (15.4), throw(), fails to allocate storage, it shall return a null pointer. Any other allocation function that fails to allocate storage shall only indicate failure by throw-ing an exception of class std::bad_alloc (18.4.2.1) or a class derived from std::bad_alloc.

      So if the actual memory allocation fails currently GCC engine will just die and all allocation checks are redundand...
      Looking for a job!
      My LinkedIn Profile

      The post was edited 1 time, last by devast3d ().

    • You should turn off C++ exceptions. Always.

      So, the defensive programmer in me says that you should always check for a NULL return from the new operator and try to handle the memory allocation error. The reality is that I don't actually do this in practice. I generally assume that memory allocations succeed. Why? Because if memory allocation fails in your game, you're completely screwed. In my experience, no amount of defensive programming will allow you to recover with anything more than the utter collapse of the program. This is especially true for console games.

      Usually, memory allocation (or something close to it, like getting a chunk from a memory pool) happens in the deep recesses of your code base. You'd have to bucket-brigade that failure all the way out to the main loop and handle the failure. Inevitably, you're going to miss some part of that chain and the game will crash in a weird place because of a NULL pointer that you don't think could possibly be NULL. That's probably worse.

      The problem gets better. Let's examine this code:

      Source Code

      1. int* pNumber = new int;
      2. GCC_ASSERT(pNumber != NULL);


      If the allocation of pNumber fails, what do you think will happen? Will the assert trigger? It will try, but it might not succeed. The assert probably tosses up a dialog box, which will try to allocate more memory.

      So really, the best you can hope for is some kind of controlled crash. This isn't the best scenario and I'm not even recommending that you do it. In a perfect world, you should gracefully handle memory allocation failures. But, in my opinion, the benefits really don't outweigh the extra code complexity, especially when you consider how fragile that failure path is.

      -Rez

    • Well, I did some testing and was actually surprised.

      Consider following code that was compiled under visual studio 2012, empty project

      Source Code

      1. #include <iostream>
      2. using namespace std;
      4. int main()
      5. {
      6. unsigned long long i = 0;
      7. try
      8. {
      9. while(true)
      10. {
      11. int* a = new /*(nothrow)*/ int[1000];
      12. i++;
      13. if (a == nullptr)
      14. {
      15. cerr << "null check caught";
      16. cin.get();
      17. exit(1);
      18. }
      19. }
      20. }
      21. catch(bad_alloc ex)
      22. {
      23. cerr << "exc caught";
      24. cin.get();
      25. exit(1);
      26. }
      28. return 0; // Unreachable
      29. }
      Display All


      With default project settings you may guess the exception is thrown and catched. BUT! If you disable exceptions in project settings then... exception IS STILL THROWN! The only way to suppress it is to uncomment nothrow after the new operator. In this case you can happily check for null pointer.

      So there are couple of ways. Add nothrow to each new operator, override (either global or class) new operator to use nothrow or register std::set_new_handler print a message and abort then.

      P.S. Now I don't really get it how all those console folks do their job writing games in C++ for the memory-bounded consoles...
      Looking for a job!
      My LinkedIn Profile

      The post was edited 1 time, last by devast3d ().

    • I use my own custom new macro which lets me hook in things like memory leak detection, so this would be easy for me to swap in.
      PC - Custom Built
      CPU: 3rd Gen. Intel i7 3770 3.4Ghz
      GPU: ATI Radeon HD 7959 3GB
      RAM: 16GB

      Laptop - Alienware M17x
      CPU: 3rd Gen. Intel i7 - Ivy Bridge
      GPU: NVIDIA GeForce GTX 660M - 2GB GDDR5
      RAM: 8GB Dual Channel DDR3 @ 1600mhz

    • No it is right grammar in English, it is implying that the game in question would be a lower priority than food or water.
      PC - Custom Built
      CPU: 3rd Gen. Intel i7 3770 3.4Ghz
      GPU: ATI Radeon HD 7959 3GB
      RAM: 16GB

      Laptop - Alienware M17x
      CPU: 3rd Gen. Intel i7 - Ivy Bridge
      GPU: NVIDIA GeForce GTX 660M - 2GB GDDR5
      RAM: 8GB Dual Channel DDR3 @ 1600mhz