Diagonal movement with fixed speed

[juiceeYay]

So I have the class below which will move to the location of the mouse after the "M" key is pressed (was originally going to use right-click, but it isn't registering my trackpad's right clicks), provided the sprite was clicked on first. Obviously I want it to be able to do this multiple times and at a fixed speed, so this presents two problems with my implementation:

1) If I tell the ship to move to a point whose x AND y coordinates are smaller than the current position, it just teleports there instantly; if the ships moves to point whose x OR y coordinate is smaller than the current position, it reaches the point in the appropriate amount of time ('timeToReach'), but then it just flies past and keeps moving on forever in that direction.

2) I am just not sure how exactly to implement fixed speed; 'moveToRightClick' will cause the ship to traverse any distance, big or small, in 'timeToReach' amount of time. How do I get it to move with a fixed speed of let's say 20 pixels a second, for example?

class Ship : public sf::Sprite
{
public:
    int posX, posY;
    bool clickedOn = false, moving = false;
    sf::Vector2i moveToPoint;
   
    Ship(int x, int y, sf::Texture &texture) : posX(x), posY(y)
    {
        setPosition(x, y);
        setTexture(texture);
        setColor( sf::Color(0,255,0) );
    }
   
    void isClickedOn(sf::Window &window)
    {
        sf::FloatRect spriteBounds = getGlobalBounds();
       
        if( sf::Mouse::isButtonPressed(sf::Mouse::Left) )
        {
            sf::Vector2i mousePos = sf::Mouse::getPosition(window);
           
            if( spriteBounds.contains(mousePos.x, mousePos.y) )
            {
                clickedOn = true;
            }
        }
    }
   
    void beginMoving(sf::Window &window, sf::Event &event)
    {
        //using sf::Event as a parameter to place in the event processing loop
        //to check if button is pressed and/or released
        sf::FloatRect spriteBounds = getGlobalBounds();
       
        if( clickedOn )
        {
            if( (event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::M) )
            {
                sf::Vector2i mousePos = sf::Mouse::getPosition(window);
               
                if( !( spriteBounds.contains(mousePos.x,mousePos.y) ) )
                {
                    clickedOn = false;
                    moving = true;
                    moveToPoint.x = mousePos.x;
                    moveToPoint.y = mousePos.y;
                }
            }
            if( sf::Mouse::isButtonPressed(sf::Mouse::Left) )
            {
                sf::Vector2i mousePos = sf::Mouse::getPosition(window);
               
                if( !( spriteBounds.contains(mousePos.x,mousePos.y) ) )
                    clickedOn = false;
            }
        }
    }
   
    void moveTo(sf::Time delta, int timeToReach)
    {
        if(moving)
        {
            sf::Vector2f currentPos = getPosition();
           
            float newX = (moveToPoint.x - posX) / timeToReach;
            float newY = (moveToPoint.y - posY) / timeToReach;
            newX *= delta.asSeconds();
            newY *= delta.asSeconds();
           
            move(newX,newY);
           
            if( (currentPos.x >= moveToPoint.x) && (currentPos.y >= moveToPoint.y) )
            {
                setPosition(moveToPoint.x, moveToPoint.y);
                moving = false;
                posX = moveToPoint.x;
                posY = moveToPoint.y;
                //if newX < 0 && newY < 0, then teleports to point instantly
                //if newX < 0 or newY < 0, then goes to point in correct time, but then keeps moving past
            }
        }
    }
}

[Ixrec]

What I see in that code has a lot of conceptual errors and architectural problems that will prevent your program from ever working properly (mixing events and real-time input, doing top-level event processing outside of the main pollEvent loop, trying to implement a fixed speed using a fixed duration, etc), so rather than trying to tweak what's there I'll start over and give you a skeleton of how this code should be organized because I think that would be a lot faster for everyone.

I'll be basing this on descriptions you've given in the previous threads about wanting Dota-style movement where pressing the movement key makes the player start moving towards the current mouse position, which they continue moving to at a fixed speed until they either reach it or are given a new destination.

Your main() should have a loop that looks something like this:

while(window.isOpen()) {
    sf::Event event;
    while(window.pollEvent(event)) {
        if(event.type == sf::Event::MouseButtonPressed) {
            if(event.mouseButton.button == sf::Mouse::Left) {
                sf::Vector2i mousePos(event.mouseButton.x, event.mouseButton.y);
                if(playerShip.contains(mousePos)) {
                    playerShip.isSelected(true); // I'll let you decide when it should be unselected
                }
            }
        } else if(event.type == sf::Event::KeyPressed) {
            if(event.key.code == sf::Keyboard::M) {
                playerShip.setMovementTarget(sf::Mouse::getPosition(window)); // if a target already exists, it gets overridden
            }
        } else // handle other events
    }

    sf::Time deltaTime = clock.restart();

    playerShip.update(deltaTime); // we'll worry about fixed timesteps later

    window.clear();
    playerShip.draw(window);
    window.display();
}
 

contains(), isSelected() and setMovementTarget() should be trivial to implement.  I'll assume the Ship class gets its draw method for free since it inherits from sf::Sprite.

The actual movement should be done entirely in the update method, and nowhere else.  It should look something like this:

void Ship::update(sf::Time deltaTime) {
    if(m_isSelected && m_movementTargetSet) {
        sf::FloatRect spriteBounds = getGlobalBounds();
        if(spriteBounds.contains(m_movementTarget)) {
            m_movementTargetSet = false; // we've arrived, so we can stop moving now
        } else {
            // the remaining movement the Ship has to do before stopping
            sf::Vector2f movement = getPosition() - m_movementTarget;

            // normalize that movement so we get a unit vector representing our current direction (we could use thor::unitVector() here)
            double length = sqrt(movement.x * movement.x + movement.y * movement.y);
            sf::Vector2f direction(movement.x / length, movement.y / length);

            // given our current direction and a fixed speed, we now move the appropriate distance for this frame
            sf::Vector2f movementStep = direction * deltaTime.asSeconds() * PLAYER_SPEED;
            setPosition(getPosition() + movementStep);
        }
    }
}
 

There are loads of corner cases and eventualities I'm not even trying to deal with here, and obviously it won't quite compile as-is since I'm writing this off the top of my head, but this is how your movement code should be structured in relation to the rest of the program.

[juiceeYay]

Would you mind elaborating on the conceptual errors and architectural problems? What exactly is wrong with using my original code (with corrections using your suggestions) as such:

int main(...)
{
    //other stuff

    while (window.isOpen())
    {
        // Process events
        sf::Event event;
        while (window.pollEvent(event))
        {
            // Close window : exit
            if (event.type == sf::Event::Closed) {
                window.close();
            }

            // Espace pressed : exit
            if (event.type == sf::Event::KeyPressed && event.key.code == sf::Keyboard::Escape) {
                window.close();
            }
           
            ship.isClickedOn(window);
            ship.beginMoving(window, event);
        }
       
        sf::Time deltaT = clock.restart();

        ship.moveTo(deltaT);

        window.clear();
        window.draw(ship);
        window.display();
    }
   
    return EXIT_SUCCESS;
}
 

The only distinction I see is that you used:

        if(event.type == sf::Event::MouseButtonPressed) {
            if(event.mouseButton.button == sf::Mouse::Left) {

Where instead I used:

        if( sf::Mouse::isButtonPressed(sf::Mouse::Left) ) {

Thanks for the idea of the unit vector, I was thinking of using the distance formula, normalization wasn't too far off I suppose haha.

[Ixrec]

- A lot of initial event handling code is in your Ship class, whereas all of it should be in the pollEvent() loop.  It's common for that event handling to involve calling methods on Ship, or set variables that are later result in calls to Ship methods, but Ship itself should not be the one checking which mouse buttons or keys are being pressed.  That's coupling your Ship entity far too tightly to your game loop/input logic.  Specifically, your Ship really doesn't need direct access to your Window and Event objects (except in the draw() method where it needs a window to draw itself on).

- You were mixing event-based input and real-time input, which is never good because those represent two fundamentally different ways of handling input that don't make sense together.  You'll see a lot of other threads on this forum where we tell newbies not to do that.  If you don't know what that means, I'm specifically referring to one of the Ship methods doing this:

if( (event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::M) ) { // event-based input
}
if( sf::Mouse::isButtonPressed(sf::Mouse::Left) ) { // real-time input
}

What you want to do is have a pollEvent() loop that deals with all of the pending events (remember there may be several), and then move on to any real-time tests you need afterward.

// event-based input
while (window.pollEvent(event)) {
    if( (event.type == sf::Event::KeyPressed) {
    } else // other types of events
}

// then real-time input, if needed
if( sf::Mouse::isButtonPressed(sf::Mouse::Left) ) { ... }

- You were using real-time input (isButtonPressed) to determine if the player's Ship was selected or not.  You almost certainly want to use event-based input if the goal is to select something in reaction to a click.  With real-time input you may miss clicks (if one frame takes too long), react to a single click multiple times (if it takes multiple frames), or any number of other strange issues.

- Arguably the most serious: Your movement logic doesn't appear to contain any attempt at calculating how much the Ship should move during this one frame (it just calls setPosition() on the final destination) and yet you were expressing surprise at the fact that "it just teleports there instantly", which indicates a serious conceptual issue of some kind.  I figured showing you an update() method that handled this properly would be the fastest way to clear up the confusion.

Those are the really big ones anyway.

[juiceeYay]

Ah that makes a lot more sense then, thank you so much. When I was using the real-time event, it would often not register a click or it would register multiple clicks for the few milliseconds I clicked.

I will have to give the distinction between real-time and event-based inputs some more study then, thanks again for all the help.