Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: How To Better Optimize This Code?  (Read 4448 times)

0 Members and 1 Guest are viewing this topic.

A_Pacifist

  • Newbie
  • *
  • Posts: 2
    • View Profile
How To Better Optimize This Code?
« on: May 12, 2016, 12:43:10 am »
Hey Guys!
   I have been using SFML for quite some time now, but I want to really focus on optimizing and using better coding practices. My question is pretty straightforward. How could I optimize the code below to look better and not be so monotonous.

                //Movement code using if statements
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))
                {
                        playerSprite.move(-4, 0);
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right))
                {
                        playerSprite.move(4, 0);
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
                {
                        playerSprite.move(0, -4);
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))
                {
                        playerSprite.move(0, 4);
                }
I am guessing I could use a switch statement or an enumeration, but I don't know how to implement this.

Thanks for any help!
Calvin

Erdrick

  • Jr. Member
  • **
  • Posts: 61
    • View Profile
    • Email
Re: How To Better Optimize This Code?
« Reply #1 on: May 12, 2016, 01:39:49 am »
Use "else if" instead of "if" on the last three so it doesnt need to check unnecessary conditions
« Last Edit: May 12, 2016, 01:41:20 am by Erdrick »

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 11028
    • View Profile
    • development blog
    • Email
AW: How To Better Optimize This Code?
« Reply #2 on: May 12, 2016, 06:23:09 am »
Adding if else changes the behavior slightly. Now you can't move up and to the right at the same time.

The code can't be really optimized more.
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://duerrenberger.dev/blog/

nicox11

  • Jr. Member
  • **
  • Posts: 51
    • View Profile
Re: How To Better Optimize This Code?
« Reply #3 on: May 12, 2016, 09:35:54 am »
It's not an optimisation, but try to use defined values (variables for example) instead of random ones (ie: your "4"). Furthermore, if you suddenly want to change your 4 by something else, you need to modify 4 lines.

It'll be easier to read your code with "Player_Speed" than just "4".

Erdrick

  • Jr. Member
  • **
  • Posts: 61
    • View Profile
    • Email
Re: AW: How To Better Optimize This Code?
« Reply #4 on: May 12, 2016, 05:19:57 pm »
Adding if else changes the behavior slightly. Now you can't move up and to the right at the same time.

The code can't be really optimized more.

thanks for mentioning this.  He mentioned switch so i thought he was accepting the change in behavior but i should have said it anyway

Mortal

  • Sr. Member
  • ****
  • Posts: 284
    • View Profile
Re: How To Better Optimize This Code?
« Reply #5 on: May 12, 2016, 05:21:46 pm »
you may try an alternative approach by using std::map or perhaps std::unordered_map or even std::vector and for-range loop, but you need to benchmark it to be sure which one is highly suitable for you.

possible implementation would be like this:
static const std::unordered_map<sf::Keyboard::Key, sf::Vector2f> keyPending{
    {sf::Keyboard::Up, sf::Vector2f{0, -4}},
    {sf::Keyboard::Down, sf::Vector2f{0, 4}},
    {sf::Keyboard::Left, sf::Vector2f{-4, 0}},
    {sf::Keyboard::Right, sf::Vector2f{4, 0}}
};

for (const auto& pair : keyPending)
{
    if (sf::Keyboard::isKeyPressed(pair.first))
        playerSprite.move(pair.second);
}
« Last Edit: May 12, 2016, 05:49:56 pm by MORTAL »

Jesper Juhl

  • Hero Member
  • *****
  • Posts: 1405
    • View Profile
    • Email
Re: How To Better Optimize This Code?
« Reply #6 on: May 12, 2016, 05:37:39 pm »
IMHO trying to micro optimize something as trivial as this is pointless. It is unlikely to take up any measurable amount of CPU time and will be completly dwarfed by other code that does work like AI, Physics, rendering etc.
This code should just be written in the most clear manner possible and don't worry about performance.

Hapax

  • Hero Member
  • *****
  • Posts: 3379
  • My number of posts is shown in hexadecimal.
    • View Profile
    • Links
Re: How To Better Optimize This Code?
« Reply #7 on: May 12, 2016, 11:39:13 pm »
The code itself seems perfectly fine and would do just the job required.

How could I optimize the code below to look better
Since this seems to be more about how your code looks than how it functions, one suggestion would be to remove all of the braces since there is only one statement based of the truth of the condition in the if statements, thus:
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))
            playerSprite.move(-4, 0);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right))
            playerSprite.move(4, 0);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
            playerSprite.move(0, -4);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))
            playerSprite.move(0, 4);
Or, more compact:
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))  playerSprite.move(-4, 0);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right)) playerSprite.move(4, 0);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))    playerSprite.move(0, -4);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))  playerSprite.move(0, 4);
Selba Ward -SFML drawables
Cheese Map -Drawable Layered Tile Map
Kairos -Timing Library
Grambol
 *Hapaxia Links*

DarkRoku12

  • Full Member
  • ***
  • Posts: 203
  • Lua coder.
    • View Profile
    • Email
Re: How To Better Optimize This Code?
« Reply #8 on: May 13, 2016, 12:42:08 am »
you may try an alternative approach by using std::map or perhaps std::unordered_map or even std::vector and for-range loop, but you need to benchmark it to be sure which one is highly suitable for you.

possible implementation would be like this:
static const std::unordered_map<sf::Keyboard::Key, sf::Vector2f> keyPending{
    {sf::Keyboard::Up, sf::Vector2f{0, -4}},
    {sf::Keyboard::Down, sf::Vector2f{0, 4}},
    {sf::Keyboard::Left, sf::Vector2f{-4, 0}},
    {sf::Keyboard::Right, sf::Vector2f{4, 0}}
};

for (const auto& pair : keyPending)
{
    if (sf::Keyboard::isKeyPressed(pair.first))
        playerSprite.move(pair.second);
}

This implementation is the same ( if the compiler unroll the loop ) or worst than the first version presented if the compiler can't unroll it or inline the lookups to the map.

And @Jesper Juhl say a very good point, try to optimize these things is unnecessary, where the AI, game logic or physics, and the draw calls are the hotspot of the game.

And you can use if\else if on up/down and left/right.

When dealing with keys, is a very good point to take into account the key rollover.
« Last Edit: May 13, 2016, 03:52:31 am by DarkRoku »
I would like a spanish/latin community...
Problems building for Android? Look here

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32498
    • View Profile
    • SFML's website
    • Email
Re: How To Better Optimize This Code?
« Reply #9 on: May 13, 2016, 07:59:03 am »
Quote
IMHO trying to micro optimize something as trivial as this is pointless. It is unlikely to take up any measurable amount of CPU time and will be completly dwarfed by other code that does work like AI, Physics, rendering etc.
This code should just be written in the most clear manner possible and don't worry about performance.
And this is exactly what the OP is asking:
Quote
better coding practices [...] the code below to look better [...] not be so monotonous
... so I think everyone talking about performances is slightly off-topic here ;)

What's important is to:
- avoid using magic numbers
- avoid repeating the same piece of code

So:
/* static const */ float playerSpeed = 4.f;
sf::Vector2f direction = {0, 0};
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))  direction.x -= 1;
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right)) direction.x += 1;
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))    direction.y -= 1;
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))  direction.y += 1;
player.move(direction * playerSpeed /* * elapsedTime */);
 
Laurent Gomila - SFML developer

Elias Daler

  • Hero Member
  • *****
  • Posts: 599
    • View Profile
    • Blog
    • Email
Re: How To Better Optimize This Code?
« Reply #10 on: May 13, 2016, 08:37:38 am »
Another thing I can note is that it's better to use another speed for diagonal movement, because otherwise the player will go faster. If player has a normalSpeed = 1.f (which is used for horizontal and vertical movement) you need to make speed.x = speed.y = sqrt(1/2) when going diagonally, so that:
speed.x ^ 2 + speed.y ^ 2 = normalSpeed
Tomb Painter, Re:creation dev (abandoned, doing other things) | edw.is | @EliasDaler

Hapax

  • Hero Member
  • *****
  • Posts: 3379
  • My number of posts is shown in hexadecimal.
    • View Profile
    • Links
Re: How To Better Optimize This Code?
« Reply #11 on: May 13, 2016, 01:59:41 pm »
I would generally use the method proposed above by Laurent.
I, personally, would prefer ++direction.x over direction.x += 1 but it's just a matter of taste.

Another thing I can note is that it's better to use another speed for diagonal movement, because otherwise the player will go faster. If player has a normalSpeed = 1.f (which is used for horizontal and vertical movement) you need to make speed.x = speed.y = sqrt(1/2) when going diagonally, so that:
speed.x ^ 2 + speed.y ^ 2 = normalSpeed
This can also be calculated using sine and cosine but since it's diagonal, they're identical. Basically, at 45 degrees, sine and cosine are at approximately 0.707 (or just 0.7) so you can adjust the diagonal speed by multiplying both x and y movement components by 0.7.
Remember to only do this if movement is occuring in horizontal and vertical direction.

This is an example of how I once did this for an SFML game jam. It determines the direction from an enumeration passed from another function but you can see that horizontal and vertical motions are (1, 0) based where diagonal motions are (angleOnAxis, angleOnAxis) based. angleOnAxis is approximately 0.7 (as explained above)
Selba Ward -SFML drawables
Cheese Map -Drawable Layered Tile Map
Kairos -Timing Library
Grambol
 *Hapaxia Links*

A_Pacifist

  • Newbie
  • *
  • Posts: 2
    • View Profile
Re: How To Better Optimize This Code?
« Reply #12 on: May 13, 2016, 10:23:29 pm »
Thanks for all the help!
I wasn't to concerned about the actual time it took to run the code, I was just concerned that there were a bunch of if statements. I was once told by a CS teacher that if you are copying and pasting code repeatedly, you're probably doing something wrong.
Thanks all!