A library for pens and brushes not working

[Cruz]

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

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

To copy to clipboard, switch view to plain text mode 

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

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

To copy to clipboard, switch view to plain text mode 

...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]

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);

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);

To copy to clipboard, switch view to plain text mode 

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]

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());

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());

To copy to clipboard, switch view to plain text mode 

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]

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

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

To copy to clipboard, switch view to plain text mode 

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

[d_stranz]

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))
{
}

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

To copy to clipboard, switch view to plain text mode 

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 );

// 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 );

To copy to clipboard, switch view to plain text mode 

[Cruz]

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));

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

To copy to clipboard, switch view to plain text mode 

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

whatever.setBrush(yellowBrush);

whatever.setBrush(yellowBrush);

To copy to clipboard, switch view to plain text mode 

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;

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

    QBrush brushYellow;
    QBrush brushRed;
    QBrush brushBlue;
};

extern ColorUtil colorUtil;

To copy to clipboard, switch view to plain text mode 

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);

whatever.setBrush(colorUtil.brushYellow);

To copy to clipboard, switch view to plain text mode 

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]

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]

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]

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());

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());

To copy to clipboard, switch view to plain text mode 

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]

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?