PDA

View Full Version : A library for pens and brushes not working



Cruz
6th October 2015, 17:07
Hello,

I find myself creating a lot of QBrush and QPen objects when I draw things on the screen or create items for a graphics scene. I decided to create a class for easy access to QBrushes like so:



class ColorUtil
{
public:
ColorUtil();
~ColorUtil(){}

QBrush brushYellow;
QBrush brushRed;
QBrush brushBlue;
};

ColorUtil::ColorUtil()
{
brushYellow.setColor(QColor::fromRgb(255,255,0,125 ));
brushRed.setColor(QColor::fromRgb(255,125,125,255) );
brushBlue.setColor(QColor::fromRgb(0,0,255,125));
}


Now when I try to use the brushes from the ColorUtil class above for example in the constructor of a graphics scene:




class PocScene : public QGraphicsScene
{
Q_OBJECT

public:

QGraphicsRectItem* car;
QGraphicsEllipseItem* lWheel;
QGraphicsEllipseItem* rWheel;

public:
PocScene(QObject *parent = 0);
~PocScene(){}
};

PocScene::PocScene(QObject *parent) : QGraphicsScene(parent)
{
setSceneRect(-1000, -1000, 2000, 2000);

double wheelRadius = 0.05;
double carHeight = 0.15;
double carWidth = 0.3;

QPen pen;
pen.setCosmetic(true);
pen.setWidth(2);

QBrush yellowBrush(QColor::fromRgb(255,255,0,125));
QBrush redBrush(QColor::fromRgb(255,125,125,255));
QBrush blueBrush(QColor::fromRgb(0,0,255,125));

ColorUtil colorUtil;

car = addRect(-0.5*carWidth, -0.5*carHeight, carWidth, carHeight, pen, yellowBrush);
lWheel = addEllipse(-wheelRadius, -wheelRadius, 2.0*wheelRadius, 2.0*wheelRadius, pen, colorUtil.brushRed); // ColorUtil used here!
rWheel = addEllipse(-wheelRadius, -wheelRadius, 2.0*wheelRadius, 2.0*wheelRadius, pen, blueBrush);
}



...it turns out that it does not work and the left wheel of the car is not colored while the right one is. I don't understand why this does not work. Printing of color values inside the ColorUtil class shows me that the QBrushes have been instantiated correctly, so what is the difference between the QBrushes inside the ColorUtil class and the locally instantiated QBrush objects that do work?

prasad_N
6th October 2015, 19:32
I am not sure of exact problem but


lWheel = addEllipse(-wheelRadius, -wheelRadius, 2.0*wheelRadius, 2.0*wheelRadius, pen, colorUtil.brushRed); // ColorUtil used here!
rWheel = addEllipse(-wheelRadius, -wheelRadius, 2.0*wheelRadius, 2.0*wheelRadius, pen, blueBrush);

If you are using these two lines, they will over lap as you are using same geometry for both wheels & you would see only 2nd wheel with overlapping color.

But if you are using them one at a time & still you have a problem, I am not sure of what is the wrong.

Cruz
6th October 2015, 19:36
Oh, when trying to make the code as simple as possible I cut away these lines:



lWheel->setParentItem(car);
lWheel->setPos(car->rect().x() + 0.5*lWheel->rect().width(), car->rect().y() - 0.5*lWheel->rect().height());

rWheel->setParentItem(car);
rWheel->setPos(car->rect().x() + car->rect().width() - 0.5*rWheel->rect().width(), car->rect().y() - 0.5*rWheel->rect().height());


The wheels don't overlap and the car looks nice. However, the left wheel is not colored and the right wheel is and I don't understand why.

prasad_N
6th October 2015, 20:09
hmm,
I could see that with the following code


PocScene::PocScene(QObject *parent) : QGraphicsScene(parent)
{
setSceneRect(-1000, -1000, 2000, 2000);

ColorUtil colorUtil;
addRect(0,0,20,20, QPen(Qt::red), QBrush(Qt::yellow));
addRect(20,20,40,40, QPen(Qt::red), colorUtil.brushYellow);
}

Sorry for the spamming but I am now waiting for some one to answer this.

d_stranz
7th October 2015, 05:30
The difference is in how you are constructing the brushes.

In your ColorUtil class, you are using the default QBrush constructor, which sets the fill style to Qt:: NoBrush. Calling setColor() after construction simply tells the QBrush to set its color, but it still has the NoBrush style. In the case of your local QBrush variables, you are calling the QBrush( const QColor & ) constructor, which has a default second argument of the brush style that in this case defaults to Qt:: SolidPattern. Changing your ColorUtil constructor to this will solve the problem:



ColorUtil::ColorUtil()
: brushYellow(QColor::fromRgb(255,255,0,125))
, brushRed(QColor::fromRgb(255,125,125,255))
, brushBlue.setColor(QColor::fromRgb(0,0,255,125))
{
}


This class is sort of a waste of time anyway - I don't know what you are trying to optimize, because the object instances you are setting the brushes on are making a copy of whatever brush you pass in, so either QBrush:: operator=() or the QBrush copy constructor is being called in any case. Each time you create a ColorUtil instance on the stack, you are in fact creating three QBrush instances, not one, but you are using only one of the three instances, and making a copy of it at that.

If you want to continue with this approach, then the way to optimize it is to make the ColorUtil class a singleton (only one of them exists in the program) created on the heap (not stack) so that you only construct your brush or pen instances once and you simply make copies of them as needed.



// ColorUtil.h

class ColorUtil
{
public:
static ColorUtil * instance()
{
if ( !sColorUtil )
sColorUtil = new ColorUtil;
return sColorUtil;
}

// You should never have public member variables, but if you're going to go down that unhappy road, here it is:
QBrush yellowBrush;
QBrush redBrush;
QBrush whateverBrush;

private:
ColorUtil();
ColorUtil( const ColorUtil & ) {}
~ColorUtil() {}

static ColorUtil * sColorUtil;
}

// in ColorUtil.cpp:
ColorUtil * ColorUtil::sColorUtil = 0;

ColorUtil::ColorUtil()
: brushYellow(QColor::fromRgb(255,255,0,125))
, brushRed(QColor::fromRgb(255,125,125,255))
, brushBlue.setColor(QColor::fromRgb(0,0,255,125))
{
}

// OtherCode.cpp
// You use this as follows:

ColorUtil * pCU = ColorUtil::instance();
whatever->setBrush( pCU->yellowBrush );

Cruz
7th October 2015, 17:06
Hi!

First of all, thank you d_stranz for solving my problem.

What I am trying to optimize is the simplicity of my drawing code. For a while now I grew tired of manually assigning colors to pens and brushes, so I ended up dragging a large block of preconstructed pens and brushes with me, such as



QBrush yellowBrush(QColor::fromRgb(255,255,0,125));
QBrush redBrush(QColor::fromRgb(255,125,125,255));
QBrush blueBrush(QColor::fromRgb(0,0,255,125));


so that later on in the code I can simply refer to



whatever.setBrush(yellowBrush);


And now I moved this block of preconstructed drawing tools (I picked up the habit of copy pasitng it between projects) into a separate ColorUtil object, that apart from readily providing pens and brushes in my favorite colors, can do some other nifty color related things too.

I am a strong believer in code simplicity and thus I will happily invest a few microseconds at construction time to construct several copies of even never used objects, if this makes the rest of my code so much easier to shape. I am also willing to break the holy commandments of OO programming, such as don't use public access variables. To shock you entirely, I have made the ColorUtil a globally accessible object like so:




struct ColorUtil
{
ColorUtil();
~ColorUtil(){}

QBrush brushYellow;
QBrush brushRed;
QBrush brushBlue;
};

extern ColorUtil colorUtil;


such that now anywhere I feel the need to draw, after including the ColorUtil class, I can go ahead and just



whatever.setBrush(colorUtil.brushYellow);


I have been using this programming paradigm now for many many years, and the prophecies of the universe ending when using this construct simply failed to come true. On the other hand, I have avoided amassing pointless setters and getters, and enjoyed simple, IDE aided code use.

Thank you again for pointing out the NoBrush problem, you have helped me on more than one occasions in the past.

Cruz

d_stranz
7th October 2015, 18:20
I have been using this programming paradigm now for many many years, and the prophecies of the universe ending when using this construct simply failed to come true.

Yet... ;)

All well and good if you are writing code for your own use. I have been doing library design and implementation for many years now, both for in-house use as well as for external licensing, and trust me, if there is a way for someone to misuse a class, they will. There is often as much code associated with handling conditions that can't possibly happen as there is in implementing the actual use cases.

Jack might understand that ColorUtil::yellowBrush is supposed to be read-only, but Fred is working on a different part of the program and doesn't like the shade yellowBrush uses, so he changes it to his liking. Now Jack suddenly finds all of his yellow things have changed color, and he doesn't have a clue why.

Glad I could be of help.

Kryzon
8th October 2015, 02:01
If you don't plan to change the brush colours after they are constructed, you can declare all those members as 'const QBrush.'
It sends a stronger message that they're read-only.

lyesqiz
12th October 2015, 08:50
Oh, when trying to make the code as simple as possible I cut away these lines:



lWheel->setParentItem(car);
lWheel->setPos(car->rect().x() + 0.5*lWheel->rect().width(), car->rect().y() - 0.5*lWheel->rect().height());

rWheel->setParentItem(car);
rWheel->setPos(car->rect().x() + car->rect().width() - 0.5*rWheel->rect().width(), car->rect().y() - 0.5*rWheel->rect().height());


The wheels don't overlap and the car looks nice. However, the left wheel is not colored and the right wheel is and I don't understand why.

i agree on this

d_stranz
12th October 2015, 19:32
i agree on this

Agree on what? That the wheels don't overlap and the car looks nice? Or that the left wheel is not colored and the right wheel is? Or that you don't understand why?