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;
}
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...