From 6d2ce1b0e7364317e06815cd47dbb1cec8037208 Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Tue, 14 Apr 2015 13:23:37 +0200 Subject: [PATCH] Timeline: Properly encapsulate render pass states We have to make sure that all the nodes get deleted when the states are deleted. Previously, we relied on the RenderState destructor to recursively delete nodes owned by their parents. This is rather hard to understand and can easily fail if we create a pass state without calling TimelineRenderState::assembleNodeTree() afterwards. The best way to deal with this is to properly encapsulate the nodes into the states and add destructors which delete them. Change-Id: I8b1ce16084afc1c85a90609f8f8d889147f7832f Reviewed-by: Joerg Bornemann --- src/libs/timeline/timelineitemsrenderpass.cpp | 111 ++++++++++++------ src/libs/timeline/timelinenotesrenderpass.cpp | 46 +++++--- .../timeline/timelineselectionrenderpass.cpp | 33 ++++-- .../qmlprofilerbindingloopsrenderpass.cpp | 86 +++++++++----- 4 files changed, 187 insertions(+), 89 deletions(-) diff --git a/src/libs/timeline/timelineitemsrenderpass.cpp b/src/libs/timeline/timelineitemsrenderpass.cpp index 41f0a362cd2..13668ebeb3e 100644 --- a/src/libs/timeline/timelineitemsrenderpass.cpp +++ b/src/libs/timeline/timelineitemsrenderpass.cpp @@ -56,16 +56,30 @@ private: QColor m_selectionColor; }; -struct TimelineItemsRenderPassState : public TimelineRenderPass::State { - TimelineItemsRenderPassState() : indexFrom(std::numeric_limits::max()), indexTo(-1) {} - int indexFrom; - int indexTo; - TimelineItemsMaterial collapsedRowMaterial; +class TimelineItemsRenderPassState : public TimelineRenderPass::State { +public: + TimelineItemsRenderPassState(const TimelineModel *model); + ~TimelineItemsRenderPassState(); + + QSGNode *expandedRow(int row) const { return m_expandedRows[row]; } + QSGNode *collapsedRow(int row) const { return m_collapsedRows[row]; } + + const QVector &expandedRows() const { return m_expandedRows; } + const QVector &collapsedRows() const { return m_collapsedRows; } + TimelineItemsMaterial *collapsedRowMaterial() { return &m_collapsedRowMaterial; } + + int indexFrom() const { return m_indexFrom; } + int indexTo() const { return m_indexTo; } + void updateIndexes(int from, int to); + void updateCollapsedRowMaterial(float xScale, int selectedItem, QColor selectionColor); + +private: + int m_indexFrom; + int m_indexTo; + TimelineItemsMaterial m_collapsedRowMaterial; QVector m_expandedRows; QVector m_collapsedRows; - const QVector &expandedRows() const { return m_expandedRows; } - const QVector &collapsedRows() const { return m_collapsedRows; } }; struct OpaqueColoredPoint2DWithSize { @@ -214,16 +228,16 @@ static void updateNodes(int from, int to, const TimelineModel *model, TimelineItemsGeometry &row = expandedPerRow[i]; if (row.usedVertices > 0) { row.allocate(&static_cast( - state->m_expandedRows[i])->material); - state->m_expandedRows[i]->appendChildNode(row.node); + state->expandedRow(i))->material); + state->expandedRow(i)->appendChildNode(row.node); } } for (int i = 0; i < model->collapsedRowCount(); ++i) { TimelineItemsGeometry &row = collapsedPerRow[i]; if (row.usedVertices > 0) { - row.allocate(&state->collapsedRowMaterial); - state->m_collapsedRows[i]->appendChildNode(row.node); + row.allocate(state->collapsedRowMaterial()); + state->collapsedRow(i)->appendChildNode(row.node); } } @@ -290,36 +304,25 @@ TimelineRenderPass::State *TimelineItemsRenderPass::update(const TimelineAbstrac TimelineItemsRenderPassState *state; if (oldState == 0) - state = new TimelineItemsRenderPassState; + state = new TimelineItemsRenderPassState(model); else state = static_cast(oldState); - float selectedItem = renderer->selectedItem() == -1 ? -1 : + int selectedItem = renderer->selectedItem() == -1 ? -1 : model->selectionId(renderer->selectedItem()); - state->collapsedRowMaterial.setScale(QVector2D(spacing / parentState->scale(), 1)); - state->collapsedRowMaterial.setSelectedItem(selectedItem); - state->collapsedRowMaterial.setSelectionColor(selectionColor); + state->updateCollapsedRowMaterial(spacing / parentState->scale(), selectedItem, selectionColor); - if (state->m_expandedRows.isEmpty()) { - state->m_expandedRows.reserve(model->expandedRowCount()); - state->m_collapsedRows.reserve(model->collapsedRowCount()); - for (int i = 0; i < model->expandedRowCount(); ++i) - state->m_expandedRows << new TimelineExpandedRowNode; - for (int i = 0; i < model->collapsedRowCount(); ++i) - state->m_collapsedRows << new QSGNode; - } - - if (state->indexFrom < state->indexTo) { - if (indexFrom < state->indexFrom) { - for (int i = indexFrom; i < state->indexFrom; + if (state->indexFrom() < state->indexTo()) { + if (indexFrom < state->indexFrom()) { + for (int i = indexFrom; i < state->indexFrom(); i+= TimelineItemsGeometry::maxEventsPerNode) - updateNodes(i, qMin(i + TimelineItemsGeometry::maxEventsPerNode, state->indexFrom), - model, parentState, state); + updateNodes(i, qMin(i + TimelineItemsGeometry::maxEventsPerNode, + state->indexFrom()), model, parentState, state); } - if (indexTo > state->indexTo) { - for (int i = state->indexTo; i < indexTo; i+= TimelineItemsGeometry::maxEventsPerNode) + if (indexTo > state->indexTo()) { + for (int i = state->indexTo(); i < indexTo; i+= TimelineItemsGeometry::maxEventsPerNode) updateNodes(i, qMin(i + TimelineItemsGeometry::maxEventsPerNode, indexTo), model, parentState, state); } @@ -332,7 +335,7 @@ TimelineRenderPass::State *TimelineItemsRenderPass::update(const TimelineAbstrac if (model->expanded()) { for (int row = 0; row < model->expandedRowCount(); ++row) { TimelineExpandedRowNode *rowNode = static_cast( - state->m_expandedRows[row]); + state->expandedRow(row)); rowNode->material.setScale( QVector2D(spacing / parentState->scale(), static_cast(model->expandedRowHeight(row))) / @@ -342,8 +345,7 @@ TimelineRenderPass::State *TimelineItemsRenderPass::update(const TimelineAbstrac } } - state->indexFrom = qMin(state->indexFrom, indexFrom); - state->indexTo = qMax(state->indexTo, indexTo); + state->updateIndexes(indexFrom, indexTo); return state; } @@ -458,4 +460,43 @@ void OpaqueColoredPoint2DWithSize::set(float nx, float ny, float nw, float nh, f r = nr; g = ng, b = nb; a = 255; } +TimelineItemsRenderPassState::TimelineItemsRenderPassState(const TimelineModel *model) : + m_indexFrom(std::numeric_limits::max()), m_indexTo(-1) +{ + m_expandedRows.reserve(model->expandedRowCount()); + m_collapsedRows.reserve(model->collapsedRowCount()); + for (int i = 0; i < model->expandedRowCount(); ++i) { + TimelineExpandedRowNode *node = new TimelineExpandedRowNode; + node->setFlag(QSGNode::OwnedByParent, false); + m_expandedRows << node; + } + for (int i = 0; i < model->collapsedRowCount(); ++i) { + QSGNode *node = new QSGNode; + node->setFlag(QSGNode::OwnedByParent, false); + m_collapsedRows << node; + } +} + +TimelineItemsRenderPassState::~TimelineItemsRenderPassState() +{ + qDeleteAll(m_collapsedRows); + qDeleteAll(m_expandedRows); +} + +void TimelineItemsRenderPassState::updateIndexes(int from, int to) +{ + if (from < m_indexFrom) + m_indexFrom = from; + if (to > m_indexTo) + m_indexTo = to; +} + +void TimelineItemsRenderPassState::updateCollapsedRowMaterial(float xScale, int selectedItem, + QColor selectionColor) +{ + m_collapsedRowMaterial.setScale(QVector2D(xScale, 1)); + m_collapsedRowMaterial.setSelectedItem(selectedItem); + m_collapsedRowMaterial.setSelectionColor(selectionColor); +} + } // namespace Timeline diff --git a/src/libs/timeline/timelinenotesrenderpass.cpp b/src/libs/timeline/timelinenotesrenderpass.cpp index ec748923887..a80756a2536 100644 --- a/src/libs/timeline/timelinenotesrenderpass.cpp +++ b/src/libs/timeline/timelinenotesrenderpass.cpp @@ -57,19 +57,26 @@ struct NotesGeometry const int NotesGeometry::maxNotes = 0xffff / 2; -struct TimelineNotesRenderPassState : public TimelineRenderPass::State +class TimelineNotesRenderPassState : public TimelineRenderPass::State { +public: TimelineNotesRenderPassState(int expandedRows); + ~TimelineNotesRenderPassState(); - QSGGeometryNode *createNode(); - - NotesMaterial material; - QSGGeometry nullGeometry; - QSGGeometryNode *m_collapsedOverlay; - QVector m_expandedRows; - + QSGNode *expandedRow(int row) const { return m_expandedRows[row]; } QSGNode *collapsedOverlay() const { return m_collapsedOverlay; } const QVector &expandedRows() const { return m_expandedRows; } + + QSGGeometry *nullGeometry() { return &m_nullGeometry; } + NotesMaterial *material() { return &m_material; } + +private: + QSGGeometryNode *createNode(); + + NotesMaterial m_material; + QSGGeometry m_nullGeometry; + QSGGeometryNode *m_collapsedOverlay; + QVector m_expandedRows; }; const QSGGeometry::AttributeSet &NotesGeometry::point2DWithDistanceFromTop() @@ -135,21 +142,21 @@ TimelineRenderPass::State *TimelineNotesRenderPass::update(const TimelineAbstrac collapsed << timelineIndex; } - QSGGeometryNode *collapsedNode = state->m_collapsedOverlay; + QSGGeometryNode *collapsedNode = static_cast(state->collapsedOverlay()); if (collapsed.count() > 0) { collapsedNode->setGeometry(NotesGeometry::createGeometry(collapsed, model, parentState, true)); collapsedNode->setFlag(QSGGeometryNode::OwnsGeometry, true); } else { - collapsedNode->setGeometry(&state->nullGeometry); + collapsedNode->setGeometry(state->nullGeometry()); collapsedNode->setFlag(QSGGeometryNode::OwnsGeometry, false); } for (int row = 0; row < model->expandedRowCount(); ++row) { - QSGGeometryNode *rowNode = static_cast(state->m_expandedRows[row]); + QSGGeometryNode *rowNode = static_cast(state->expandedRow(row)); if (expanded[row].isEmpty()) { - rowNode->setGeometry(&state->nullGeometry); + rowNode->setGeometry(state->nullGeometry()); rowNode->setFlag(QSGGeometryNode::OwnsGeometry, false); } else { rowNode->setGeometry(NotesGeometry::createGeometry(expanded[row], model, parentState, @@ -162,20 +169,27 @@ TimelineRenderPass::State *TimelineNotesRenderPass::update(const TimelineAbstrac } TimelineNotesRenderPassState::TimelineNotesRenderPassState(int numExpandedRows) : - nullGeometry(NotesGeometry::point2DWithDistanceFromTop(), 0) + m_nullGeometry(NotesGeometry::point2DWithDistanceFromTop(), 0) { - material.setFlag(QSGMaterial::Blending, true); + m_material.setFlag(QSGMaterial::Blending, true); m_expandedRows.reserve(numExpandedRows); for (int i = 0; i < numExpandedRows; ++i) m_expandedRows << createNode(); m_collapsedOverlay = createNode(); } +TimelineNotesRenderPassState::~TimelineNotesRenderPassState() +{ + qDeleteAll(m_expandedRows); + delete m_collapsedOverlay; +} + QSGGeometryNode *TimelineNotesRenderPassState::createNode() { QSGGeometryNode *node = new QSGGeometryNode; - node->setGeometry(&nullGeometry); - node->setMaterial(&material); + node->setGeometry(&m_nullGeometry); + node->setMaterial(&m_material); + node->setFlag(QSGNode::OwnedByParent, false); return node; } diff --git a/src/libs/timeline/timelineselectionrenderpass.cpp b/src/libs/timeline/timelineselectionrenderpass.cpp index dbf33a8b070..343811a980f 100644 --- a/src/libs/timeline/timelineselectionrenderpass.cpp +++ b/src/libs/timeline/timelineselectionrenderpass.cpp @@ -40,6 +40,7 @@ QSGSimpleRectNode *createSelectionNode() QSGSimpleRectNode *selectionNode = new QSGSimpleRectNode; selectionNode->material()->setFlag(QSGMaterial::Blending, false); selectionNode->setRect(0, 0, 0, 0); + selectionNode->setFlag(QSGNode::OwnedByParent, false); QSGSimpleRectNode *selectionChild = new QSGSimpleRectNode; selectionChild->material()->setFlag(QSGMaterial::Blending, false); selectionChild->setRect(0, 0, 0, 0); @@ -47,12 +48,16 @@ QSGSimpleRectNode *createSelectionNode() return selectionNode; } -struct TimelineSelectionRenderPassState : public TimelineRenderPass::State { - QSGSimpleRectNode *m_expandedOverlay; - QSGSimpleRectNode *m_collapsedOverlay; +class TimelineSelectionRenderPassState : public TimelineRenderPass::State { +public: + TimelineSelectionRenderPassState(); + ~TimelineSelectionRenderPassState(); QSGNode *expandedOverlay() const { return m_expandedOverlay; } QSGNode *collapsedOverlay() const { return m_collapsedOverlay; } +private: + QSGSimpleRectNode *m_expandedOverlay; + QSGSimpleRectNode *m_collapsedOverlay; }; TimelineRenderPass::State *TimelineSelectionRenderPass::update( @@ -67,17 +72,14 @@ TimelineRenderPass::State *TimelineSelectionRenderPass::update( TimelineSelectionRenderPassState *state; - if (oldState == 0) { + if (oldState == 0) state = new TimelineSelectionRenderPassState; - state->m_expandedOverlay = createSelectionNode(); - state->m_collapsedOverlay = createSelectionNode(); - } else { + else state = static_cast(oldState); - } QSGSimpleRectNode *selectionNode = static_cast(model->expanded() ? - state->m_expandedOverlay : - state->m_collapsedOverlay); + state->expandedOverlay() : + state->collapsedOverlay()); QSGSimpleRectNode *child = static_cast(selectionNode->firstChild()); int selectedItem = renderer->selectedItem(); @@ -147,4 +149,15 @@ TimelineSelectionRenderPass::TimelineSelectionRenderPass() { } +TimelineSelectionRenderPassState::TimelineSelectionRenderPassState() : + m_expandedOverlay(createSelectionNode()), m_collapsedOverlay(createSelectionNode()) +{ +} + +TimelineSelectionRenderPassState::~TimelineSelectionRenderPassState() +{ + delete m_collapsedOverlay; + delete m_expandedOverlay; +} + } // namespace Timeline diff --git a/src/plugins/qmlprofiler/qmlprofilerbindingloopsrenderpass.cpp b/src/plugins/qmlprofiler/qmlprofilerbindingloopsrenderpass.cpp index b75e0ca5032..059d39dbe85 100644 --- a/src/plugins/qmlprofiler/qmlprofilerbindingloopsrenderpass.cpp +++ b/src/plugins/qmlprofiler/qmlprofilerbindingloopsrenderpass.cpp @@ -40,17 +40,27 @@ public: BindingLoopMaterial(); }; -struct BindingLoopsRenderPassState : public Timeline::TimelineRenderPass::State { - BindingLoopsRenderPassState() : indexFrom(std::numeric_limits::max()), indexTo(-1) {} - BindingLoopMaterial material; - int indexFrom; - int indexTo; +class BindingLoopsRenderPassState : public Timeline::TimelineRenderPass::State { +public: + BindingLoopsRenderPassState(const QmlProfilerRangeModel *model); + ~BindingLoopsRenderPassState(); - QVector m_expandedRows; + BindingLoopMaterial *material() { return &m_material; } + void updateIndexes(int from, int to); + + int indexFrom() const { return m_indexFrom; } + int indexTo() const { return m_indexTo; } + + QSGNode *expandedRow(int row) const { return m_expandedRows[row]; } const QVector &expandedRows() const { return m_expandedRows; } - - QSGNode *m_collapsedOverlay; QSGNode *collapsedOverlay() const { return m_collapsedOverlay; } + +private: + QVector m_expandedRows; + QSGNode *m_collapsedOverlay; + BindingLoopMaterial m_material; + int m_indexFrom; + int m_indexTo; }; struct Point2DWithOffset { @@ -111,14 +121,14 @@ void updateNodes(const QmlProfilerRangeModel *model, int from, int to, for (int i = 0; i < model->expandedRowCount(); ++i) { BindlingLoopsGeometry &row = expandedPerRow[i]; if (row.usedVertices > 0) { - row.allocate(&state->material); - state->m_expandedRows[i]->appendChildNode(row.node); + row.allocate(state->material()); + state->expandedRow(i)->appendChildNode(row.node); } } if (collapsed.usedVertices > 0) { - collapsed.allocate(&state->material); - state->m_collapsedOverlay->appendChildNode(collapsed.node); + collapsed.allocate(state->material()); + state->collapsedOverlay()->appendChildNode(collapsed.node); } int rowHeight = Timeline::TimelineModel::defaultRowHeight(); @@ -163,25 +173,20 @@ Timeline::TimelineRenderPass::State *QmlProfilerBindingLoopsRenderPass::update( return oldState; BindingLoopsRenderPassState *state; - if (oldState == 0) { - state = new BindingLoopsRenderPassState; - state->m_expandedRows.reserve(model->expandedRowCount()); - for (int i = 0; i < model->expandedRowCount(); ++i) - state->m_expandedRows << new QSGNode; - state->m_collapsedOverlay = new QSGNode; - } else { + if (oldState == 0) + state = new BindingLoopsRenderPassState(model); + else state = static_cast(oldState); - } - if (state->indexFrom < state->indexTo) { - if (indexFrom < state->indexFrom) { - for (int i = indexFrom; i < state->indexFrom; + if (state->indexFrom() < state->indexTo()) { + if (indexFrom < state->indexFrom()) { + for (int i = indexFrom; i < state->indexFrom(); i += BindlingLoopsGeometry::maxEventsPerNode) updateNodes(model, i, qMin(i + BindlingLoopsGeometry::maxEventsPerNode, - state->indexFrom), parentState, state); + state->indexFrom()), parentState, state); } - if (indexTo > state->indexTo) { - for (int i = state->indexTo; i < indexTo; i+= BindlingLoopsGeometry::maxEventsPerNode) + if (indexTo > state->indexTo()) { + for (int i = state->indexTo(); i < indexTo; i+= BindlingLoopsGeometry::maxEventsPerNode) updateNodes(model, i, qMin(i + BindlingLoopsGeometry::maxEventsPerNode, indexTo), parentState, state); } @@ -191,8 +196,7 @@ Timeline::TimelineRenderPass::State *QmlProfilerBindingLoopsRenderPass::update( parentState, state); } - state->indexFrom = qMin(state->indexFrom, indexFrom); - state->indexTo = qMax(state->indexTo, indexTo); + state->updateIndexes(indexFrom, indexTo); return state; } @@ -354,6 +358,32 @@ void Point2DWithOffset::set(float nx, float ny, float nx2, float ny2) x = nx; y = ny; x2 = nx2; y2 = ny2; } +BindingLoopsRenderPassState::BindingLoopsRenderPassState(const QmlProfilerRangeModel *model) : + m_indexFrom(std::numeric_limits::max()), m_indexTo(-1) +{ + m_collapsedOverlay = new QSGNode; + m_collapsedOverlay->setFlag(QSGNode::OwnedByParent, false); + m_expandedRows.reserve(model->expandedRowCount()); + for (int i = 0; i < model->expandedRowCount(); ++i) { + QSGNode *node = new QSGNode; + node->setFlag(QSGNode::OwnedByParent, false); + m_expandedRows << node; + } +} + +BindingLoopsRenderPassState::~BindingLoopsRenderPassState() +{ + delete m_collapsedOverlay; + qDeleteAll(m_expandedRows); +} + +void BindingLoopsRenderPassState::updateIndexes(int from, int to) +{ + if (from < m_indexFrom) + m_indexFrom = from; + if (to > m_indexTo) + m_indexTo = to; +} } }