Arch Linux

NunchuckBradley

Member

Registered: 2019-12-09

Posts: 6

Learning C++. Best practices for moving sprite?

I am learning C++ and I am doing a basic sprite character movement thing with sfml graphics.

I am able to make the character move using the controls and the sprite also changes correctly.

void changeSprite(float dt, int row, int column = 0){
			// time in seconds to change animation.
			if(spriteTime > 0.3 || spriteRow != row){
				spriteRow = row;
				int height = 48;
				int width = 32;
				spritePos.width = width;
				spritePos.height = height;
				int top = (row*height)-height;
				int left = (column*width)-width;
				spritePos.top = top;
				if(column == 0){
					if(spritePos.left == (3*width)){
	                                	spritePos.left = 0;
	                        	}
	                        	else{
	                        	        spritePos.left += width;
	                        	}
				}
				else{
					spritePos.left = left;
				}
				sprite.setTextureRect(spritePos);
				spriteTime = 0;
			}
			else{
				spriteTime += dt;
			}
		}

this code is the sprite visual change which is a member of a class. dt is from

float dt = clock.restart().asSeconds();

and in the while loop window.isOpen() also is below, to make the character move.

if(sf::Keyboard::isKeyPressed(sf::Keyboard::Up)){
			player.changeSprite(dt, 4);
			player.move(dt, 'u');
}

I am concerned that the practices used are bad. I set spriteTime and spriteRow multiple times and I feel like changing variables every 0.0002 seconds isnt good practice.

like the

int height

, if i have that outside the function, is it better for a computer then to have it inside the function?

If this were to run on an old system, would it have any effects maybe? Please tell me im wrong or help me get better. Thanks in advance

Last edited by NunchuckBradley (2020-06-11 20:42:35)

chaseleif

Member

From: Texas

Registered: 2020-08-01

Posts: 18

Re: Learning C++. Best practices for moving sprite?

My thoughts:

Declaring the height (and width and top) inside of that if statement makes them local to that if statement, reference to those variables are invalid outside of the if statement.
If you are referencing those variables declared local to the 'if' elsewhere, and it isn't breaking, then your compiler is extrapolating what you want to do, or you are getting the value of another variable with the same name, and that is not correct. If you need these values outside of the if, then they should be declared outside of the if.
It is better to keep the scope of things smaller, e.g. not having every variable as a global. When something has a larger scope then it could be referenced from a larger body of code and thus may be required to be available to a larger body of code. Limited scopes are better, it allows the compiler to more easily recognize where a variable may still be in-use or live for scheduling of what registers do what or what needs to go into memory. This makes it easier for the compiler to plan for what values will be needed where.
So, if you expected to use these outside of the if statement then you declared them in the wrong place.
If you don't need them outside of the if, then the separate declaration of those variables are useless, and (as hinted at just above) something a compiler optimization would likely get rid of altogether. You could have just assigned the spritePos members instead of introducing new variables, and compiler optimizations could do just that. The only declaration in that function that makes sense (from the flow you posted) is the "left" variable, because the spritePos member left is conditionally assigned, though you could also get rid of that variable and use a conditional assignment instead. Further, if you wanted to keep these variables, for readability or whatnot, since their values never change they could be constants.

Computers are good at doing lots of things fast. The downside to performing constant computation is creating competition for CPU time, if there are other processes that also want to do work.
That said, the code you've shown doesn't call player.changeSprite unless the isKeyPressed(up) returns true.
If you are using builtin functions from an API to refresh the windows, they should have internally implemented a means to ensure they allow other processes to get some CPU time. This is probably something you don't need to worry about yet if you aren't having problems. You can check out resources on operating system scheduling and look up information about threads (affinity, scheduling, contention) to get an idea of how multiple programs are running at the same time.

I'm not sure how whatever you're using does it, but consider the refresh rate of a monitor, a TV, or the FPS of a video. A video with 32 frames-per-second will appear to have continuous motion. So, 1 second divided by 32 gives you a frequency, that you don't need to exceed, with which you can refresh any series of still images to make them appear continuous ... Human perception is limited and we can take advantage of this when making motion, compressing images, etc.

I haven't used the sf::Time before. A good practice is to look at docs, especially when using unfamiliar functions, I look at docs even for functions I know how to use.
I found these:
https://www.sfml-dev.org/documentation/ … _1Time.php
https://www.sfml-dev.org/documentation/ … 1Clock.php

Since you are using units smaller than seconds, you should use the millisecond function so you can change your float comparisons to integer comparisons. It is better to work with integers unless you actually need a float.
On a related note, assigning a float to equal an integer, such as 1, will give you a float that either equals 0.9999.... or 0.10000... it will be some number that is very close to 1, but it is not the same as an integer whose value is exact.
Floating points have rounding errors and you should expect them.

In the if (up pressed) block of code, you call player.changeSprite( ) followed by player.move( ).
The changeSprite method executes one portion of code if spriteTime > 0.3 or spriteRow != 4, at the end of this block spriteTime is set to zero ... otherwise it increments spriteTime by dt.
I am assuming that the starting row is 4. The OR condition will allow the if block to execute regardless of the value of spriteTime as long as the row is not 4. I don't know if this is what you intend.
Immediately after that player.move( ) is called, regardless of which path the changeSprite function took and what happened to the spriteTime.
Depending on what happens in the move there could be unintended consequences from this, so it could be better to bring that condition higher up, as in: if keypress(up) { if (time>.3) { call the two functions } else { } }