Thread: Trouble fixing a critical bug in my WormMove algorithm!

  1. #16
    Registered User
    Join Date
    Apr 2012
    Posts
    19
    @iMalc - haha, yea I had that thought pop into my head. But it would have to be a bit more controlled

    Here is WormBody.h and "my" game loop class

    Game.cpp
    Game.h
    WormBody.h

  2. #17
    Registered User
    Join Date
    Apr 2012
    Posts
    19
    When I had my body in a vector, it broke every time it's actual size increased. This creates a new array inside the container so I thought that maybe copying of the segments might be breaking the tail.

    I added this code to see if creating a new deque for the body would break the tail. This would happen every time I press space.
    Code:
        if (Game::GetInput().IsKeyDown(sf::Key::Space))//TEST
        {
            temp = _body;
            _body.~deque();
            _body = temp;
        }
    What resulted is that it broke when I had 1000 segments consistently , with 100 it only happen some of the time and when I had only 10 I couldn't get it to break at all.

    Maybe that clue helps out.

  3. #18
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    Code:
    temp = _body;
    _body.~deque();
    _body = temp;
    >_<

    That's so extremely wrong I fear it may break the U|\|1\/3r$3.

    Yog Sothoth!

    Do j00Z $33 \/\/|-|@ j00Z have done!?

    '/0U'\/3 bR0|<3|\| 7|-|3 3|\|71R3 U|\|1\/3R53.

    Seriously though, a destructor ends the life of an object. You can't use the assignment operator on a dead object.

    Soma

  4. #19
    Registered User
    Join Date
    Apr 2012
    Posts
    19
    Errrrrr...... im not sure about the internals of deque but isn't the objects in _body copied into temp (ie they are now seperate objects) so that the destructor is destroying the objects in _body not temp which will be copied into _body again!

  5. #20
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    It is never okay to call the destructor directly like that.

    To do what you want, you should be using:
    Code:
    {
        std::deque<MyType> temp = _body;
        std::swap(temp, _body);
    }
    This performs the copying once and only once, and has no undefined (universe breaking) behaviour.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  6. #21
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    im not sure about the internals of deque but isn't the objects in _body copied into temp (ie they are now seperate objects) so that the destructor is destroying the objects in _body not temp which will be copied into _body again!
    It has nothing to do with the internal implementation. It has everything to do with one simple fact: destructors end the life of an object.

    You are not entirely correct about the destructor destroying the objects of `_body'. The destructor destroys `_body' itself which includes the objects that it owns. You can't use a facility of a dead object.

    Look at it this way, you are saying "Click the "Okay" button." after you've already turned off your computer. That isn't a thing that you can do.

    To do what you want, you should be using
    You should prefer an unnamed "temp swap" instead.

    Code:
    std::deque<int>().swap(_body);
    That does the same thing and kills off the temporary "instantly" without introducing extra scope.

    Soma

  7. #22
    Registered User
    Join Date
    Apr 2012
    Posts
    19
    Cool, that for the info. I'll make sure to never do that again. (I don't want to break the U|\|1\/3r$3 )

    Now I played around with the elapsedTime in the function that calls the update of all the objects that exist in the game world. If I increase the elaspsedTime then the tail broke quicker. Normally, it is worked out by using a SFML function that gets the time from the last frame meaning that it changes each iteration of the game loop. As it seemed that it was lagging just before the breaking maybe the increased time was a factor. So assuming that it is true and a larger elaspedTime value causes the error, I now have to work out why.

    Does this sound logical or should I be looking at something else?

  8. #23
    Registered User
    Join Date
    Apr 2012
    Posts
    19
    Actually, I have some more news. I was looking at what grumpy was saying and decided I should look into the inner loop of this function.
    Code:
    void Worm::WormMove(float elapsedTime)
    {
        int counter = 0;
        int max =0;
        //create move vector for the Worm head
        move(elapsedTime);
    
        //move all the body parts
        for (std::deque<WormBody>::iterator it = _body.begin(); it != _body.end(); it++)
        {
             counter = 0;
             float timeLeft = elapsedTime;
            while (it->MoveBody(timeLeft)) //time is referenced so it will decrease each iteration
            {
                counter++;
                std::vector<WormPart::MoveMessage> msg = it->DeleteMsg();
                if (it + 1 != _body.end()) 
                    (it+1)->SendMsg(msg);
            }
            if (counter > max)
                max = counter;
        }
        std::cout << "DEBUG MESSAGE: counter is " << max << "\n";
    }
    What I noticed by printing the counter is that when it breaks the counter spikes really high. Quite interesting!!!

    Edit: fixed a error in the code
    Last edited by Dogged; 07-25-2012 at 06:44 AM.

  9. #24
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    Look at your class hierarchy:
    Code:
    GameObject
       |
     Sprite    ...
       |        |
    WormBody  Sprite
       |      /
      WormPart
    WormPart inherits from Sprite twice. I'm not sure what that will do, but it's probably not what you meant. I'd get rid of the ": public Sprite" part of
    Code:
    class WormPart : public Sprite
    {
    public:
    ...
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

  10. #25
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    Quote Originally Posted by phantomotap View Post
    You should prefer an unnamed "temp swap" instead.

    Code:
    std::deque<int>().swap(_body);
    That does the same thing and kills off the temporary "instantly" without introducing extra scope.
    Even better.
    I just hope he can understand what that's doing just as well.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  11. #26
    Registered User
    Join Date
    Apr 2012
    Posts
    19
    oogabooga, you confused me!!!

    My class hierarchy is meant to be like this
    Code:
    GameObject
       |
    
     Sprite   
    
       |      
    
      WormPart  
    
       |      \  
    
    WormBody  Worm
    so WormPart should inherent from Sprite but I don't see how WormBody is inheriting directly from Sprite but through WormPart.

    Maybe I am blind, so here is the header for WormBody again.

    Code:
    class WormBody : public WormPart
    {
    public:
        WormBody(float X, float Y, float angle, int colour);
        ~WormBody();
        WormBody(const WormBody& copy);
    
        void Update(float elapsedTime);
        bool MoveBody(float& elapsedTime);
    private:
        void CreatePolygon();
    };

  12. #27
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    Hmmm, you're absolutely right. I don't know what code I was looking at! Sorry about that. I'm giving your code a read-through right now. Will let you know if I find something.
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

  13. #28
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    I read through your code (while watching The Daily Show and The Cobert Report) and all I noticed was that you misspelled "calculate" in CalualteGridLocations and CalulateBodyRotation.

    Oh, and CheckMoveMsg should have a "return false" at the end (otherwise it will sometimes not set it's return value properly).

    Also, you might want to check that elapsedTime never goes negative after this line:
    Code:
     *elapsedTime = *elapsedTime - timeTaken;
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

  14. #29
    Registered User
    Join Date
    Apr 2012
    Posts
    19
    How embarrassing that I misspelled calculate twice, opps! Anyway, thanks for reading my code oogabooga. I did notice the missing return false line and added it about a day ago and using a cout statement I have never seen it become negative. For now I just made elapsedTime zero if it is negative but that is not a perfect fix.

    Looking at my data while debugging, what is happening is that the messages gets "stuck" on a body piece as a message doesn't get sent across to the next piece and as a result it keeps checking against a message which it has already passed. Leaving all the other messages to built up like a blockage in a pipe.

    Now why is message is getting stuck...duno. It could be the functions that delete and send this data that are causing this bug. But what I still think, is that the first function I posted is causing this bug.
    I Just read this website and maybe i'm having an error to do with this funny float business. I don't have time right now to look into this as I got a lecture starting but I will get onto it. Anyway, it's something to look into

    Here is the website: Comparing Floating Point Numbers, 2012 Edition | Random ASCII



  15. #30
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    In your isPointOnLineSeg function, shouldn't the first case in your outermost if/else if/...
    structure be put last? Otherwise it grabs every case where lineVec .x and .y are both not
    exactly zero. The test for both non-zero is to protect against division by zero, but since
    it's the first case it gets chosen more than it should.


    As a test, the implementation below seems fairly inefficient, but you could try it as a replacement
    just to see what happens.

    Code:
    // The line segment is point a to b. The point is c.
    // The distance from a to b is compared to the sum of the distances from a to c and from b to c.
    bool AAH::math::isPointOnLineSeg(sf::Vector2f a, sf::Vector2f b, sf::Vector2f c)
    {
        float distAB = (a.x - b.x) * (a.x - b.x) + (a.y - b.y) * (a.y - b.y);
        float distAC = (a.x - c.x) * (a.x - c.x) + (a.y - c.y) * (a.y - c.y);
        float distBC = (b.x - c.x) * (b.x - c.x) + (b.y - c.y) * (b.y - c.y);
        return AreSame(distAB, distAC + distBC);
    }
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Trouble with an algorithm to find edges
    By kbrown51 in forum C++ Programming
    Replies: 3
    Last Post: 08-09-2011, 02:51 PM
  2. Trouble Understanding Structure Algorithm
    By JJohnson in forum C Programming
    Replies: 14
    Last Post: 10-19-2010, 01:27 AM
  3. Trouble Generating Game Algorithm
    By justlivelife15 in forum Game Programming
    Replies: 3
    Last Post: 06-13-2007, 09:58 AM
  4. Errors i'm having trouble fixing
    By yaniv89 in forum Windows Programming
    Replies: 5
    Last Post: 08-26-2005, 02:35 PM
  5. Trouble W/Getline Algorithm
    By drdroid in forum C++ Programming
    Replies: 7
    Last Post: 02-26-2003, 09:39 AM