I want to develop my own signal scope in C++. So I use qt for this purpose. I add a graphicsView and a push Button in ui. At connected method to push button I update the graphicsView(finally I'll pass this method to a thread). Each time that I press push button, Despite the deleting pointer the usage of memory is increase. How should I control this?
I check memory usage in vs15 diagnostic tool.
c++ header file:
#pragma once
#include <QtWidgets/QMainWindow>
#include "ui_QtGuiApplication.h"
#include <QGraphicsScene>
#include <QGraphicsPathItem>
class QtGuiApplication : public QMainWindow
{
Q_OBJECT
public:
QtGuiApplication(QWidget *parent = Q_NULLPTR);
private:
Ui::QtGuiApplicationClass ui;
QGraphicsScene* scene = new QGraphicsScene();
QPolygon pol;
QGraphicsPathItem* pathItem = new QGraphicsPathItem();
int index_ = 0; // graph offset
QPen* myPen = new QPen(Qt::red, 2);
private slots:
void btn_Display_clicked();
};
c++ source file:
#include "QtGuiApplication.h"
#include <math.h> /* sin */
#define pi 3.14159265
QtGuiApplication::QtGuiApplication(QWidget *parent)
: QMainWindow(parent)
{
ui.setupUi(this);
ui.graphicsView->setScene(scene);
ui.graphicsView->show();
connect(ui.btn_Display, SIGNAL(clicked()), this, SLOT(btn_Display_clicked()));
}
void QtGuiApplication::btn_Display_clicked()
{
scene->removeItem(pathItem);
QPoint pos;
double x_amp_, y_amp_;
int x_pix_, y_pix_;
double phi_ = 0;
double freq_ = 5;
for (int i = 0; i<800; i++)
{
y_amp_ = 100 * sin(2 * pi*freq_*i / 811 + phi_) + 20 * index_;
x_amp_ = i;
x_pix_ = x_amp_;
y_pix_ = y_amp_;
pos.setX(x_pix_);
pos.setY(y_pix_);
pol.append(pos);
}
QPainterPath* myPath = new QPainterPath();
(*myPath).addPolygon(pol);
pathItem = scene->addPath(*myPath, *myPen);
delete myPath;
pol.clear();
index_ = (index_ + 1) % 20; // just for sense of change in graph
}
I have several comments.
You're doing a lot of perfectly unnecessary manual memory management. Why is it bad? Because if you weren't, your bug would be impossible by construction. All of your uses of manual memory allocation (explicit new) are unnecessary. Hold the objects by value: they were designed to work that way, and let the compiler worry about the rest.
You're declaring objects in scopes that are too big. You declare the polygon and pen in the class even though they belong in the scope of btn_Display_clicked; you declare the loop variables (x_amp_ and y_amp_) outside of the loop.
The connect statement is redundant: setupUi does the connection for you.
You're not fully leveraging C++11.
Includes of the form <QtModule/QClass> postpone detection of project misconfiguration until link time and are unnecessary. You're meant to include <QClass> directly. If you get a compile error, then your project is misconfigured - perhaps you need to re-run qmake (typical solution).
In C++, <cmath> is idiomatic, not <math.h>. The latter is retained for compatibility with C.
Perhaps you might wish to use M_PI. It is a widely used name for pi in C++.
Preallocating the polygon avoids a premature pessimization of reallocating the point storage as it grows.
You're losing precision by using an integer-based polygon, unless that's a choice you made for a specific reason. Otherwise, you should use QPolygonF.
Thus, the interface becomes:
#pragma once
#include <QMainWindow>
#include <QGraphicsScene>
#include <QGraphicsPathItem>
#include "ui_QtGuiApplication.h"
class GuiApplication : public QMainWindow
{
Q_OBJECT
public:
GuiApplication(QWidget *parent = {});
private:
Ui::GuiApplicationClass ui;
QGraphicsScene scene;
QGraphicsPathItem pathItem; // the order matters: must be declared after the scene
int index_ = 0;
Q_SLOT void btn_Display_clicked();
};
And the implementation:
#include "GuiApplication.h"
#define _USE_MATH_DEFINES
#include <cmath>
#if !defined(M_PI)
#define M_PI (3.14159265358979323846)
#endif
GuiApplication::GuiApplication(QWidget *parent) : QMainWindow(parent)
{
ui.setupUi(this);
ui.graphicsView->setScene(&scene);
ui.graphicsView->show();
pathItem.setPen({Qt::red, 2});
scene.addItem(&pathItem);
}
void GuiApplication::btn_Display_clicked()
{
const double phi_ = 0;
const double freq_ = 5;
const int N = 800;
QPolygonF pol{N};
for (int i = 0; i < pol.size(); ++i) {
qreal x = i;
qreal y = 100 * sin(2 * M_PI * freq_ * i / 811 + phi_) + 20 * index_;
pol[i] = {x, y};
}
QPainterPath path;
path.addPolygon(pol);
pathItem.setPath(path);
index_ = (index_ + 1) % 20; // just for sense of change in graph
}
In C++14 you could easily factor out the generator if you wish to reuse it elsewhere; then the loop would be replaced with:
auto generator = [=,i=-1]() mutable {
++i;
return QPointF{qreal(i), 100 * sin(2 * M_PI * freq_ * i / 811 + phi_) + 20 * index_};
};
std::generate(pol.begin(), pol.end(), generator);
Alas, as it stands, the btn_Display_clicked implementation throws away the heap allocated to store the polygon, and it unnecessarily copies data from the polygon to the internal element storage within the path. We can avoid both by modifying the painter item's path directly:
void GuiApplication::btn_Display_clicked()
{
const double phi_ = 0;
const double freq_ = 5;
const int N = 800;
if (pathItem.path().isEmpty()) { // preallocate the path
QPainterPath path;
path.addPolygon(QPolygon{N});
pathItem.setPath(path);
}
auto path = pathItem.path();
pathItem.setPath({}); // we own the path now - item's path is detached
Q_ASSERT(path.elementCount() == N);
for (int i = 0; i < path.elementCount(); ++i) {
qreal x = i;
qreal y = 100 * sin(2 * M_PI * freq_ * i / 811 + phi_) + 20 * index_;
path.setElementPositionAt(i, x, y);
}
pathItem.setPath(path);
index_ = (index_ + 1) % 20; // just for sense of change in graph
}
Because QPainterPath is an implicitly shared value class, the path = item.path(); item.setPath({}); sequence is equivalent to moving the path out of the path item. The subsequent setPath(path) moves it back, since we don't do further changes to the local copy.