Threading problems.

[MW2TopTenWORLD]

Ok, before you say it I am OK with C++ but when it comes to pointers, threads, multi-d arrays and other things I am noob... I know they're important but I haven't learned it as of now...

Well I am using sf::Thread not std::Thread

The problem is I am "pausing" the thread but it jut pauses the main thread!

I Will Post my code below please help .

This file is called mainScreen.cpp it represents my Main Menu.
The problem is with the thread that changes the bottom message color.
(If I call it without the sf::sleep or as not being a thread the colors just change instantaneously (obviously))

#include "mainScreen.h"
#include <SFML/Graphics.hpp>
#include <SFML/Window.hpp>
#include "variables.h"
#include "textCreator.h"
#include "Funcs.h"
#include <dos.h>
#include <SFML/System.hpp>

/*NOTE: sf::Vector2f = (width, height)*/

void changeBtmMessageTextColor(int x);
void scrollBtmMessage();

sf::RectangleShape playBTN;
sf::Text playBTNText;
sf::Texture mainMenuTexture;
sf::Sprite mainMenuImg;
sf::Text creator;
int bottomMsgColor = 1; //1 = green, 2 = yellow, 3 = red
sf::Thread scroller(&scrollBtmMessage);
sf::Thread colorScroller(&changeBtmMessageTextColor, bottomMsgColor);

void changeBtmMessageTextColor(int x) //this is the block of code with problems!
{
        if(x==1)
        {
                creator.setColor(sf::Color::Yellow);
                sf::sleep(sf::milliseconds(1500));
        }
        if(x==2)
        {
                creator.setColor(sf::Color::Red);
                sf::sleep(sf::milliseconds(1500));
        }
        if(x==3)
        {
                creator.setColor(sf::Color::Green);
                sf::sleep(sf::milliseconds(1500));
        }
        sf::sleep(sf::milliseconds(1500));
}

void scrollBtmMessage()
{
        if(creator.getPosition().x >= fullscreen.width){
                creator.setPosition(0-3500,creator.getPosition().y);
        }
        else{
                creator.setPosition(creator.getPosition().x+2,creator.getPosition().y);
        }
}

void mainScreen::drawMainScreen(sf::RenderWindow *window)
{
        isMainScreenOn=true;
        if(isMainScreenBuilt == false)
        {
                //playBTN
                playBTN.setSize(sf::Vector2f((fullscreen.width*0.35),(fullscreen.height*0.10)));  
                playBTN.setPosition((funcs::findCenterOfScreenWidth(fullscreen.width)) - ((fullscreen.width*0.35)/2),fullscreen.height*0.45);
                playBTN.setFillColor(sf::Color::White);

                //playBTNText
                //textCreator::createText(playBTNText, mainFont, sf::Color::Green, "New Game", ((fullscreen.width*0.35)/2), ((fullscreen.height*0.10)/2));
                playBTNText.setColor(sf::Color::Black);
                playBTNText.setFont(mainFont);
                playBTNText.setString("Play Game");
                playBTNText.setPosition(funcs::findCenterOfScreenWidth(fullscreen.width)-90,(fullscreen.height*0.45)+((fullscreen.height*0.10)/PI));

                //mainMenuImg
                mainMenuTexture.loadFromFile("Images\\main_menu_img.png");
                mainMenuImg.setTexture(mainMenuTexture);
                mainMenuImg.setPosition(funcs::findCenterOfScreenWidth(fullscreen.width)-(1000/2),fullscreen.height*0.05);

                //creator
                creator.setColor(sf::Color::Green);
                creator.setFont(mainFont);
                creator.setString("Made by : MW2TopTenWORLD a.k.a MW2T1999 || Subscribe me at http://www.youtube.com/mw2toptenworldmodz (Click this message to go there!) || Thank You For Purchasing my Game or whatever...");
                creator.setPosition(0, fullscreen.height-(fullscreen.height*0.05));
               

                isMainScreenBuilt = true;
        }
        window->draw(playBTN);
        window->draw(playBTNText);
        window->draw(mainMenuImg);
        window->draw(creator);
        colorScroller.launch();
        //changeBtmMessageTextColor(bottomMsgColor);
        scroller.launch();
        if(bottomMsgColor!=3){
                bottomMsgColor++;
        }else{
                bottomMsgColor=1;
        }
}
 

Please Help

[zsbzsb]

• Threads are evil.
• Threads contain many pitfalls.
• You don't even try to protect your data, let alone fixing the pitfalls that threads bring.
• Don't use threads.
• Forget you ever heard of threads.
• Forget you ever heard of the concept of multithreading.
• Make your program single threaded until you have performance issues.
• If you have performance issues use a profiler to find the bottleneck and then optimize.
• Goto's are evil...
• Goto 7;
• Unhandled stack-overflow exception.

[Ixrec]

Did you read the SFML tutorial on threads? http://sfml-dev.org/tutorials/2.1/system-thread.php

1) No globals.  Globals are bad.  I have no idea what a global sf::Thread would do, but that might be part of your problem.

2) There is no reason to use threads here.  At all.  You should have a main game loop where you update the state of all your entities and then draw all of them.  The scrolling/color changing logic should be done by those update functions, not by creating/switching/sleeping threads (which are not only far more expensive but also less precise and much easier to screw up).

3) Arguably part of #2, but you probably have loads of race conditions.

4) As you probably already know, std::thread is just better.

5) We can't see your main() function, so we have no idea how often drawMainScreen() gets called.  If it only gets called once, then you only draw() these things one time in the entire program (regardless of what the threads do), so maybe you've mistaken the lack of redrawing for "pausing the main thread."  If you call drawMainScreen() every frame, then you're launching two new threads every frame...which is suicidal.  Either way, the design of that function is just plain wrong.

6) Like zsbzsb said, avoid threads at all costs.  Very, very few people can use them effectively, and it'll be a very long time before you get in a situation where there's even any potential benefit to using them.

Hopefully that's enough to help you understand where you went wrong.


Totally unrelated, but:

int bottomMsgColor = 1; //1 = green, 2 = yellow, 3 = red

Go read about enums.  They're much better than magic numbers like this.

[MW2TopTenWORLD]

@Ixrec I will make program single threaded right now and btw I do know about Enums, I tried writing one before I used that "alternative" but I forgot how it worked, so I wrote the "alternative" and then went back to "Jumping Into C++" and had a refresh on my concept of enums but then I just decided I would leave that instead of an enum to save my time.

@zsbzsb Thanks!

[Jesper Juhl]

I agree with everything that others have said so far.

Some additional points:

void changeBtmMessageTextColor(int x) //this is the block of code with problems!
{
   if(x==1)
   {
...
   }
   if(x==2)
   {
...
   }
   if(x==3)
   {
...
   }
   sf::sleep(sf::milliseconds(1500));
}

Since "x" can't have the values 1, 2 & 3 at the same time there's no need to test for all 3 - use "else if".

void scrollBtmMessage()
{
   if(creator.getPosition().x >= fullscreen.width){
      creator.setPosition(0-3500,creator.getPosition().y);
   }

zero minus some magic number?? Why?

Also, magic numbers are evil and confusing and if they appear more than once they tend to not be updated everywhere when code changes. Make a named constant instead - that conveys far more meaning, like;

 namespace {
   const int something = 3500;
 }

 static const int something_else = -3500;
 

or similar (but don't use a macro "#define foo bar" - macros have a special place in C++ hell).

                bottomMsgColor++;

Not really relevant for this type, but as a general rule you should stick to prefix increment "++stuff;" rather than postfix increment "stuff++;" unless you really need the semantics of postfix increment, since prefix is never slower and can be faster for some types.

[Jesper Juhl]

I do know about Enums, I tried writing one before I used that "alternative" but I forgot how it worked

If you are reading up on them anyway and use a C++11 capable compiler, then read up on strongly typed enums "enum class" rather than old-school enums.

[MW2TopTenWORLD]

@Jesper Juhl thank you for those tips! Those are some kinds of stuff that don't come in books... (except the else if ones)

And for your new post about enums, sadly I can't use C++11 ;(.
I was using Visual Studio 2013 in the beginning (which I hope is C++11 capable) but now I had to go to 2010 because I am using sfTheora for my game intro video

[MW2TopTenWORLD]

Also for however requested my main loop here is the Drawing part of it.

...
window.clear();
                if(introVideo.isDone() == false)
                {
                        window.draw(introVideo);
                        window.draw(pressENT);
                }
                if(introVideo.isDone() == true)
                {
                        if(isMainScreenOn==true)
                        {
                                mainScreen::drawMainScreen(&window);
                        }
                }
        window.display();
    }
    return EXIT_SUCCESS;
}

[Jesper Juhl]

Also for however requested my main loop here is the Drawing part of it.

So basically you just clear(), draw() and display() once and then terminate the program...?

At the very least you'd want some kind of loop. Like a basic event loop (modified version of your code):

while (window.isOpen()) {
    while (window.pollEvent(event)) {
        switch (event.type) {
        // ... do event related stuff ...
        }
    }
    window.clear();
    if (introVideo.isDone()) {
        if (isMainScreenOn) {
            mainScreen::drawMainScreen(&window);
        }
    } else {
        window.draw(introVideo);
        window.draw(pressENT);
    }
    window.display();
}
 

[MW2TopTenWORLD]

@Jesper Juhl that was just the last bit of the loop
Here is the whole main.cpp code

#include <sfTheora.h>
#include <SFML/Audio.hpp>
#include <SFML/Config.hpp>
#include <SFML/Graphics.hpp>
#include <SFML/Network.hpp>
#include <SFML/OpenGL.hpp>
#include <SFML/System.hpp>
#include <SFML/Window.hpp>
#include "textCreator.h"
#include "mainScreen.h"
#include <iostream>
#include <cstdlib>
#include <time.h>
#include <string>
#include "variables.h"
#include "Funcs.h"

using namespace std;

int main()
{
        sf::ContextSettings settings;
        settings.antialiasingLevel = 8;
        sf::RenderWindow window(sf::VideoMode(fullscreen.width, fullscreen.height), "SFML App", sf::Style::Fullscreen, settings);
        sf::Clock clock;

        window.setVerticalSyncEnabled(true);
       
        mainFont.loadFromFile("Fonts\\sansation.ttf");

        sf::Text pressENT;
        pressENT.setColor(sf::Color::White);
        pressENT.setFont(mainFont);
        pressENT.setString("Press [ENTER] to Skip this Video");
        pressENT.setPosition(fullscreen.width-(fullscreen.width*0.65), fullscreen.height-(fullscreen.height*0.10));

        sftheora::Video introVideo("intro.ogg");
        //introVideo.setPosition((fullscreen.width/2)+(1280/2),0);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();

                        if (event.type == sf::Event::KeyPressed)
                        {
                                switch(event.key.code)
                                {
                                case sf::Keyboard::Escape:
                                        {
                                                window.close();
                                                break;
                                        }
                                case sf::Keyboard::F12:
                                        {
                                                sf::Image screen = window.capture();
                                                srand(time(0));
                                                long double randomNum = 0;
                                                for(int i = 0; i < 5; i++)
                                                {
                                                        randomNum = rand() % 125000000 + 1;
                                                }
                                                string fileName = "Screenshots\\" + std::to_string(randomNum) + ".png";
                                                screen.saveToFile(fileName);
                                                break;
                                        }
                                case sf::Keyboard::Return:
                                        {
                                                if(introVideo.isDone() == false)
                                                {
                                                        introVideo.seek(introVideo.getDuration());
                                                }
                                                break;
                                        }
                                }
                        }
                        if(event.type == sf::Event::MouseButtonPressed)
                        {
                                switch(event.mouseButton.button)
                                {
                                case sf::Mouse::Left:
                                        {
                                                if((introVideo.isDone()==true)&&(isMainScreenOn==true))
                                                {
                                                        //This here is the "PlayBTN" Button :)
                                                        if((sf::Mouse::getPosition().x >= ((funcs::findCenterOfScreenWidth(fullscreen.width)) - (fullscreen.width*0.35)/2))
                                                                && (sf::Mouse::getPosition().x <= ((funcs::findCenterOfScreenWidth(fullscreen.width)) - (fullscreen.width*0.35)/2) + (fullscreen.width*0.35))
                                                                && (sf::Mouse::getPosition().y >= (fullscreen.height*0.45))
                                                                && (sf::Mouse::getPosition().y <= (fullscreen.height*0.45) + (fullscreen.height*0.10)))
                                                        {
                                                                //MessageBox(NULL, "you're in the button!", "congrats nigger!", MB_OK || MB_ICONINFORMATION); - TEST PURPOSES -
                                                                /*1 - Set Menu as OFF so it stops being drawn.
                                                                  2 - Launch a whole new ton of shit that I'll think of later...*/
                                                                isMainScreenOn = false;
                                                        }

                                                        //The Bottom Message
                                                        if((sf::Mouse::getPosition().x >= 0)
                                                                && (sf::Mouse::getPosition().x <= fullscreen.width)
                                                                && (sf::Mouse::getPosition().y <= fullscreen.height)
                                                                && (sf::Mouse::getPosition().y >= (fullscreen.height-(fullscreen.height*0.05))))
                                                        {
                                                                ShellExecute(NULL, "open", "http://www.youtube.com/mw2toptenworldmodz", NULL, NULL, NULL);
                                                        }
                                                }
                                        }
                                }
                        }
                               
        }

                introVideo.update(clock.restart());

        window.clear();
                if(introVideo.isDone() == false)
                {
                        window.draw(introVideo);
                        window.draw(pressENT);
                }
                if(introVideo.isDone() == true)
                {
                        if(isMainScreenOn==true)
                        {
                                mainScreen::drawMainScreen(&window);
                        }
                }
        window.display();
    }
    return EXIT_SUCCESS;
}

Before you say so YES I know about rand() being evil and me not should be using the SFML Events for the button presses and stuff like that but its my first game its very simple I Don't need to be very cautious... but I am taking "notes" of all that in my head for a more future serious project

[Ixrec]

Might as well comment on a few other things I see there since we seem to be in code review mode

- I don't think all of those #includes are necessary. SFML/Network in particular I don't think you're using.
- Actually, there's nothing wrong with using SFML events for button presses.  If you expect your GUI needs to go beyond these primitive buttons you should probably look at things like SFGUI, but for just this it's fine.
- The logic to check if the mouse is inside a button can probably be made far more clear and concise with the right use of sf::Rect::contains().
- Things like fullscreen.height*0.45 probably shouldn't be in the button click detection logic.  It would be cleaner to have a tiny Button class that contains these coordinates, and then you simply ask the Button for its bounding rectangle when processing click events.
- Right now your main loop has only two states (video, main screen) but already it looks a little clunky checking which state you're in.  If you think you'll ever add a third state, it might be a good idea now to use an enum like GameState { VIDEO, MAINGAME }; so you can do switch(gameState) { case VIDEO: ... }, thereby clearly enforcing the small number of finite valid states your main loop can have.

[MW2TopTenWORLD]

@Ixrec thank you for that I will use most of those stuff from now on, specially the sf::Rect::contains() one

Please understand that this is my first game EVER and this is all to new to me.. I mean SFML is kind of user friendly (yay) but the way you "manage" your game is.... just new to me, C++ by it self is just not the language for a beginner but then all this things that you need to keep track from x.x.


Well again thanks for that, it might look like not but they're really helping me

Oh and btw "fullscreen.height * 0.45" is not a "magic" number or something like that... it's because that is the defined y (?) position of one of the buttons so I was telling the "mouse" to look in that way.... And I get exactly what you mean... I mean theres "nothing" pretty much "done" so far and I already feel SOOOOOO Scrambled :S sometimes I even wonder if I'm doing it right

Thanks!

[Jesper Juhl]

Staying in code review mode, here are some more observations in addition to lxrec's comments:

   mainFont.loadFromFile("Fonts\\sansation.ttf");

Get in the habit of using forward slashes "/" for file paths.
1. You don't have to escape them.
2. They work on all operating systems that SFML supports.

                        srand(time(0));

Besides the fact that you really should use the facilities available in the <random> header, why are you seeding the random number generator anew each time the user presses F12?

                        long double randomNum = 0;
                        for(int i = 0; i < 5; i++)
                        {
                            randomNum = rand() % 125000000 + 1;
                        }

Why bother initializing "randomNum" to "0" when you always assign to it inside the loop anyway?
And why on earth do you use a loop here? why is the fifth number you get from rand() (which is what you'll always end up with) better than any of the previous four?

                        string fileName = "Screenshots\\" + std::to_string(randomNum) + ".png";
                        screen.saveToFile(fileName);

Why not name your screenshots sequentially - like; screenshots/0.png, screenshots/1.png, screenshots/n.png etc?  If it's to avoid overwriting existing files, then you could just generate a unique prefix for each sequence when the program starts (and check that it really is unique before writing any files).

            if (event.type == sf::Event::KeyPressed)
...
            if(event.type == sf::Event::MouseButtonPressed)

            if (event.type == sf::Event::KeyPressed)
...
            else if (event.type == sf::Event::MouseButtonPressed)

 

It can't be both types at once.

                                ShellExecute(NULL, "open", "http://www.youtube.com/mw2toptenworldmodz", NULL, NULL, NULL);

This is very much non-portable and will only work on Windows. Why would you want to tie your program to a specific platform? I would assume that (at least part of) the reason for using SFML would be to easily gain some platform independance/portability...?

And as lxrec already said, encapsulating some of this stuff (like the different states) in classes would make the whole thing a lot more readable.

[Ixrec]

Oh and btw "fullscreen.height * 0.45" is not a "magic" number or something like that... it's because that is the defined y (?) position of one of the buttons so I was telling the "mouse" to look in that way....

Oh yeah, I wasn't calling that a magic number.  It was pretty clear you meant that the button should be situated at 45% of the screen height.  The suggestion was simply to move that information out of the click event handling.

Please understand that this is my first game EVER and this is all to new to me..

Yep, that's why we're nitpicking so many of these things.  Most of them don't matter in a 100 line toy program but knowing these things adds up to so much more productivity in the long run.

I mean SFML is kind of user friendly (yay) but the way you "manage" your game is.... just new to me, C++ by it self is just not the language for a beginner but then all this things that you need to keep track from x.x.

Yeah, thinking in terms of "game loops" and all the other game programming idioms does take a lot of getting used to.  Personally I probably learned most of it from lurking on this forum for far too long, but recently I saw Jesper link http://gameprogrammingpatterns.com/ which turned out to be a great free online book that covers a fairly significant subset of these things.  Reading it might save you some time.

Well again thanks for that, it might look like not but they're really helping me

Actually, you made it pretty obvious that you are taking our advice seriously, which is why we keep posting more of it. =)



By the way, have you tried compiling sftheora with VS2013?  Just because the libraries haven't been updated in a while doesn't mean the source is unbuildable (...or is it?).  Admittedly setting up proper builds can be a pain so maybe that's something to try after you've gotten this thing past the "proof of concept" stage...

Edit: Damn, I forgot to mention the backslashes in paths.  I knew I forgot something.

[MW2TopTenWORLD]

@Jesper Juhl , @Ixrec thank you for all of these usefull tips! (I always say this xD)

About the / instead of \\ I HAD NO IDEA!!

About the "ShellExecute" I plan my game just to be for Windows, sure my future games would be cool to Multi Platform but this one just for Windows, but thanks for that as well.

The Screenshot thing is ... far from being finished, It's just a "place holder" for now but your tips will help a lot when I "finish" it.

Thanks again for the if to else if tip

Thanks for the suggestion for that book I will read it when I have time

And I tried compiling the sfTheora source in Visual Studio... in CMAKE.... etc and it just gives me errors :\ I'm sure its compilable but I just can't do it, and Visual Studio 2010 is going to do just fine for now specially since this game is more of like of a "hello world" project.