Advertisement

SFML Cannot get sprite rect to change

Started by February 05, 2016 11:16 PM
6 comments, last by Alberth 8 years, 10 months ago

Hi everyone, so i'm trying to get a animation by moving the rectangle the the sprite has selected every frame while a certain key is pressed.

My first step was getting the rectangle to actually change when I press a key, and then I can limit it later. Unfortunately, I cannot get there. If anyone could help that would be great.

I'm posting this in a hurry as i'm about to leave. My code is here:

http://pastebin.com/LsWr5kDM

if this should be posted somewhere else, I apologize, the mistake is because i'm in a hurry to leave and didn't have a whole lot of time to look around!

Thx to anyone who so much as looks, anything is appreciated.

Don't work alone, and don't live alone. "Solitude vivifies; Isolation kills" - Joseph Roux.

C++ isn't my strength, but I don't see anywhere where you update the position of the rectangle on the sprite sheet. I'm assuming that you intend to do that in your moveTextureRect functions, where you update the Player's position but then set the textureRect for playerSprite with the playerRect object, which never changes.

-------R.I.P.-------

Selective Quote

~Too Late - Too Soon~

Advertisement

Khaiy is correct.

When you do this:


int moveTextureRectX(int diffX)
{
    playerPos.x += diffX;
           
    playerSprite.setTextureRect(playerRect); //<-- You never changed "playerRect"
    return playerPos.x;
}

...you never actually changed 'playerRect'.

Further, you have multiple ways to store positions and multiple ways to store sizes. That's not good. There should only be one variable to store one concept. You don't want to redundantly store the same concept (position or size) in multiple unrelated variables.

Also, longer more-descriptive function and variable names are better. "cPos" is not a good function name. "move()" or "changePos()" or even "changePosition()" are better names. Don't hyper-abbreviate.

On another note, this kind of function is not good:


int getTextureRectValues(int x)
{
    if (x == 1)
    {
        return playerPos.x;
    }
    else
    {
        return playerPos.y;
    }
}

The function should return return a single variable, not different variables depending on what is passed in.

The function should just return the player position directly:


vvvvvvvvvvvv
sf::Vector2i getTextureRectValues()
{
    return playerPos;
}
You may want to use an enum for directions and make a changeDir() function that will accept a direction as its argument.
class Player {
  //blah blah
  enum Direction : int { DOWN = 0, LEFT, RIGHT, UP };
  Direction facing = DOWN;
  const int TEXTURE_CELL_HEIGHT = 24;

  void changeDir(Direction dir) {
    if(dir != facing) {
      facing = dir;
      sf::IntRect srcRect = playerSprite.getTectureRect();
      srcRect.y = dir * TEXTURE_CELL_HEIGHT;
      playerSprite.setTextureRect(srcRect);
    }
  }

    //...
    if (sf::Keyboard::isKeyPressed(sf::Keyboard::W))
    {
      changeDir(UP);
      player.cPos(0, -10);   
    }
    //...

  //blah blah
}

Also, the class is named 'Player', so you don't have to name it's members 'playerThis' and 'playerThat'. You can just use 'this' and 'that', since they're already scoped within Player.
void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

You may want to use an enum for directions and make a changeDir() function that will accept a direction as its argument.


class Player {
  //blah blah
  enum Direction : int { DOWN = 0, LEFT, RIGHT, UP };
  Direction facing = DOWN;
  const int TEXTURE_CELL_HEIGHT = 24;

  void changeDir(Direction dir) {
    if(dir != facing) {
      facing = dir;
      sf::IntRect srcRect = playerSprite.getTectureRect();
      srcRect.y = dir * TEXTURE_CELL_HEIGHT;
      playerSprite.setTextureRect(srcRect);
    }
  }

    //...
    if (sf::Keyboard::isKeyPressed(sf::Keyboard::W))
    {
      changeDir(UP);
      player.cPos(0, -10);   
    }
    //...

  //blah blah
}

Also, the class is named 'Player', so you don't have to name it's members 'playerThis' and 'playerThat'. You can just use 'this' and 'that', since they're already scoped within Player.

Thanks for this!, I actually didn't know I could do that when I started this project, but after school started last year, about august, I had learned that in one of my computer science classes. It's been almost a year since I actually put this project down to continue my studies, and coming back to it i've seen a lot of things I need to change; I would like to get it working before I go back and polish it with my new knowledge.

The enum based movement is another thing I actually was meaning to implement before I put the project down. I actually have a note telling myself to look into it in my notebook, now that I know how, it should be a quick fix, but first, a working product.
EDIT:

I should make a note that the animation thing is actually required by what i'm doing this for. It's the next step up in the ladder of skills I wanted to make myself learn and improve on, otherwise everything does work, and I could start polishing now, but this is the last thing in the box that's actually required in my MVP.

Don't work alone, and don't live alone. "Solitude vivifies; Isolation kills" - Joseph Roux.

C++ isn't my strength, but I don't see anywhere where you update the position of the rectangle on the sprite sheet. I'm assuming that you intend to do that in your moveTextureRect functions, where you update the Player's position but then set the textureRect for playerSprite with the playerRect object, which never changes.

This was my original thought. I wasn't 100% sure, and I'm grateful that you were able to confirm this with me. I'm gonna rattle my brain some more now that I am home, and hopefully kick this out first thing tonight.

Don't work alone, and don't live alone. "Solitude vivifies; Isolation kills" - Joseph Roux.

Advertisement


Khaiy is correct.

When you do this:
int moveTextureRectX(int diffX)
{
playerPos.x += diffX;

playerSprite.setTextureRect(playerRect); //<-- You never changed "playerRect"
return playerPos.x;
}
...you never actually changed 'playerRect'.

Further, you have multiple ways to store positions and multiple ways to store sizes. That's not good. There should only be one variable to store one concept. You don't want to redundantly store the same concept (position or size) in multiple unrelated variables.

A lot of this project was me actually learning and experimenting with SFML. Now that im tons more familiar I'll clean this code up and get everything set straight once I get this working.


Also, longer more-descriptive function and variable names are better. "cPos" is not a good function name. "move()" or "changePos()" or even "changePosition()" are better names. Don't hyper-abbreviate.

I do agree 100%, I made this code about a little over a 6 months or so, the more I think about it, the further back it seems to go, but I had to put this down to continue school, and I've learned more in my computer science classes, and will definitely change that when I make it look pretty.


On another note, this kind of function is not good:
int getTextureRectValues(int x)
{
if (x == 1)
{
return playerPos.x;
}
else
{
return playerPos.y;
}
}
The function should return return a single variable, not different variables depending on what is passed in.

This was never ment to stay in, I was trying to seperate them to make sure other things were working, and it was a test function I ment to erase, and am doing so after I type this :P.

Don't work alone, and don't live alone. "Solitude vivifies; Isolation kills" - Joseph Roux.

Don't worry too much about the code. The fact that you read your own code, and think "what a mess" is a good sign. It means you have improved, and know better now :)

This topic is closed to new replies.

Advertisement