0

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 
}
M.Hu
  • 121
  • 1
  • 14
  • 2
    I don't see why you need pointers at all for `myPath` as you add it *by value* to `scene`? – Galik Feb 28 '18 at 00:57
  • The code calls `scene->addPath`. That would likely add to the size of the `scene`. – Bo Persson Feb 28 '18 at 00:58
  • each time I want to erase previous path and plot new path. so I think I have to add it by value. is there any other way? – M.Hu Feb 28 '18 at 01:06
  • with the line code `scene->removeItem(pathItem)` in beginning of method I think the added item is removed. – M.Hu Feb 28 '18 at 01:13
  • Your code doesn't compile - you seem to be missing `"ui_QtGuiApplication.h"`. It would be better to provide a single-file [mcve]. – Toby Speight Feb 28 '18 at 09:40
  • Please [edit] your question to show us what kind of debugging you've done. I expect you to have run your [mcve] within Valgrind or a similar checker, and to have investigated with a debugger such as GDB, for example. Ensure you've enabled a full set of compiler warnings, too. What did the tools tell you, and what information are they missing? And read Eric Lippert's [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). – Toby Speight Feb 28 '18 at 09:41

2 Answers2

3

In

pathItem = scene->addPath(*myPath, *myPen);

A new QGraphicsPathItem was created and a pointer returned to pathItem. Sooner or later the button is clicked again and

scene->removeItem(pathItem);

removes the QGraphicsPathItem from the scene. Unfortunately according to the documentation

The ownership of item is passed on to the caller (i.e., QGraphicsScene will no longer delete item when destroyed).

Deletion of pathItem is up to the programmer. pathItem not deleted and is leaked at the subsequent call to

pathItem = scene->addPath(*myPath, *myPen);

Solution: Delete pathItem before it is leaked.

scene->removeItem(pathItem);
delete pathItem; 

should do the job.

user4581301
  • 33,082
  • 7
  • 33
  • 54
2

I have several comments.

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

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

  3. The connect statement is redundant: setupUi does the connection for you.

  4. You're not fully leveraging C++11.

  5. 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).

  6. In C++, <cmath> is idiomatic, not <math.h>. The latter is retained for compatibility with C.

  7. Perhaps you might wish to use M_PI. It is a widely used name for pi in C++.

  8. Preallocating the polygon avoids a premature pessimization of reallocating the point storage as it grows.

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

Kuba hasn't forgotten Monica
  • 95,931
  • 16
  • 151
  • 313
  • Thanks for your good advice. In the future, I'll get data from LAN with UDP protocol. so the math part of this code just for test. the pixel variable in real work must be scaled with amplitude. so I have to put them in loop. I am Telecommunication engineer and beginner in C++ and Qt programming. I will contemplate about your tips. thanks. – M.Hu Feb 28 '18 at 20:24