Making a "Paint" Application

[Paki Programmer]

Ok, so I decided to try and make a MS Paint like application in which you can draw using different brushes, colors, shapes, etc. I just wanted someone to look at my code and tell me if I'm headed in the right direction with this or if I need to approach the "drawing" aspect differently or something. The code is a little bit messy and unorganized because I typed it up quickly just to get a rough idea of where to start. Ill eventually put everything in classes and encapsulate it so worry not Any feed back is appreciated

Here's the code:

#include <SFML/Graphics.hpp>
#include <iostream>
#include <ctime>
#include <list>

float x, y;

int r, g, b;

sf::RenderWindow window(sf::VideoMode(800, 600), "Paint Application");

std::list<sf::CircleShape> circles;
std::list<sf::CircleShape>::iterator iter;

void add(sf::CircleShape shape)
{
        circles.push_back(shape);
}

void update()
{
        iter = circles.begin();

        while (iter != circles.end())
        {
                window.draw(*iter);
                iter++;
        }

        //std::cout << circles.size() << std::endl;
}

int main()
{
        sf::Event ev;

        //srand((unsigned int) time(0));

        while (window.isOpen())
        {
                while (window.pollEvent(ev))
                {
                        switch (ev.type)
                        {
                        case sf::Event::Closed:
                                {
                                        window.close();
                                }
                                break;

                        case sf::Event::KeyPressed:
                                {
                                        switch (ev.key.code)
                                        {
                                        case sf::Keyboard::Escape:
                                                {
                                                        window.close();
                                                }
                                                break;
                                        }
                                }
                                break;
                        }
                }

                if (sf::Mouse::isButtonPressed(sf::Mouse::Left))
                {
                        x = (float) sf::Mouse::getPosition((window)).x;
                        y = (float) sf::Mouse::getPosition(window).y;

                        r = 255;
                        g = 0;
                        b = 255;

                        sf::CircleShape point(2);
                        point.setPosition(x, y);
                        point.setFillColor(sf::Color(r, g, b));
                        add(point);
                }

                if (sf::Mouse::isButtonPressed(sf::Mouse::Right))
                {
                        x = (float) sf::Mouse::getPosition((window)).x;
                        y = (float) sf::Mouse::getPosition(window).y;

                        r = 0;
                        g = 215;
                        b = 255;

                        sf::CircleShape point(2);
                        point.setPosition(x, y);
                        point.setFillColor(sf::Color(r, g, b));
                        add(point);
                }

                if (sf::Mouse::isButtonPressed(sf::Mouse::Middle))
                {
                        x = (float) sf::Mouse::getPosition((window)).x;
                        y = (float) sf::Mouse::getPosition(window).y;

                        r = 0;
                        g = 0;
                        b = 0;

                        sf::CircleShape point(2);
                        point.setPosition(x, y);
                        point.setFillColor(sf::Color(r, g, b));
                        add(point);
                }

                window.clear();
                update();
                window.display();
        }
       
        return 0;
}

 

[Nexus]

Very important: Declare variables as locally as possible.

For example, global variables are not necessary. x, y, r, g, b ought to be declared immediately before the point of usage. Iterators can be declared directly inside the for loop:

for (auto iter = circles.begin(); iter != circles.end(); ++iter)

Same for the event variable, you don't need to reuse it, so put its declaration before the pollEvent() call. The circle list can be passed as an argument to the functions where it is needed.

Furthermore:

• Prefer static_cast over C-style casts, since they are more expressive and less dangerous (they only convert what you expect).
  
• add() does nothing more than forward one function, I would remove it.
  
• The three if clauses at the end are very similar, consider outsourcing commonalities to a function.
• Generally, use const references instead of copies for parameters (in C++11 however, pass by value gets interesting if you exploit move semantics).
• update() has a misleading name, call it drawCircles().

Instead of managing a big list of circles, an alternative consists of using sf::RenderTexture to store the drawn image. But I'm not sure if a clear() call is required to work correctly, otherwise you could just not clear the window...

[Paki Programmer]

I don't think you saw in my original post that this was just a quick rough draft Nexus :p I'm well aware of all the things you listed and I plan on implementing them in the final version of the program but for now I just made it as simple as I could to see if I could get a semi working program. I do appreciate the input though

[Marcus]

As Nexus stated, don't create a massive list of circles. It's inefficient and can be done much better.

The easy way to do it is to have a single circle object that can be resized and recolored as needed. Then when the user clicks, you set the center of the circle to the position of the mouse and then draw the circle for as long as the user clicks. When the user stops clicking, you can move it to a position off-screen and then stop calling the draw command on it.

Don't call clear(), this will keep the drawn image on the screen instead of having to draw a bunch of circles. You can implement an interface that the user can use to change colors/brushes but make sure it gets drawn over the drawing space. Alternatively you can place it in a separate window.

[Nexus]

I don't think you saw in my original post that this was just a quick rough draft Nexus :p

Yes, I saw it, but you wouldn't really have been slower when writing directly good code, so this doesn't count