Add nodeEdit mode #46

Sedictious · 2019-08-25T16:54:55Z

This adds most of the features of node edit mode:

Right click on a node to remove it
Left click to add a node (left clicking on a node still opens up the respective inspector)
Drag any node to move it
If a selected node is dragged, then every selected node will move along with it

cardinot · 2019-08-25T22:40:00Z

src/gui/gridview.cpp

@@ -100,7 +100,7 @@ void GridView::paintFrame(QPainter& painter) const
    }
 }

-Node GridView::findNode(const QPointF& pos) const
+Node GridView::findNode(const QPointF& pos)


why removing the const?

Because we may want to move the node by changing it's coordinates, so const would throw a compilation error. selectNode wouldn't work, since we may want to move the node without adding it to our current selection. Maybe it would make more sense to rename findNode to getNode?

nope, any "find" or "get" function should be const
it seems that you are mixing two different concepts here: 1) const function, 2) const return

any getter/finder function should be const because it should never ever change a member variable

could you revert this change please? (i.e., make this function const)

cardinot · 2019-08-25T22:43:47Z

src/gui/basegraphgl.cpp

@@ -97,6 +98,7 @@ void BaseGraphGL::setup(AbstractGraph* abstractGraph, AttributesScope nodeAttrsS
    m_nodeAttrsScope = nodeAttrsScope;
    setupInspector();
    updateCache();
+    m_attrs = m_abstractGraph->node(0).attrs();


it should consider the case in which m_abstractGraph has no nodes

Is called only after the graph model is initialized so it shouldn't be a problem since we already check whether the number of nodes is not zero. But this must change if we allow to add nodes before generating a graph model

nope, this function can also be called with a null abstractGraph; in this scenario, this call would cause a crash. For instance, see BaseGraphGL::slotRestarted():

evoplex/src/gui/basegraphgl.cpp

Line 298 in 28fe85f

setup(nullptr, AttributesScope());

could you fix this line please? (i.e., only assign the attrs() if m_abstractGraph is not null and if !m_abstractGraph->nodes().empty())

cardinot · 2019-08-25T22:49:03Z

src/gui/basegraphgl.cpp

+    updateCache();
+}
+
+void BaseGraphGL::createNode(QPointF pos)


const QPointF& pos

cardinot · 2019-08-25T22:49:11Z

src/gui/basegraphgl.cpp

+    updateCache();
+}
+
+void BaseGraphGL::deleteNode(QPointF pos)


const QPointF& pos

cardinot · 2019-08-25T22:49:47Z

src/gui/basegraphgl.cpp

+    updateCache();
+}
+
+void BaseGraphGL::moveNode(Node& node, QPointF pos) {


const QPointF& pos

cardinot · 2019-08-25T22:52:22Z

src/gui/graphview.cpp

@@ -101,7 +101,7 @@ CacheStatus GraphView::refreshCache()
    return CacheStatus::Ready;
 }

-Node GraphView::findNode(const QPointF& pos) const
+Node GraphView::findNode(const QPointF& pos)


cardinot · 2019-08-25T23:02:16Z

src/gui/graphview.cpp

+                selectedStar.edges.push_back({ep.second, line});
+            }
+        }
+        selectedStar.edges.shrink_to_fit();


it's not an efficient approach; we should not be computing things in a "draw" function
actually, we should compute the coordinates only when the object is selected (or when something changes in the view)
(note that m_selectedStars is not being used after your change --- why?)

I'm still looking into this (and that's why I didn't remove m_selectedStars)

While m_selectedNodes is automatically updated when moving a node, m_selectedStars is not, since it doesn't hold a pointer to the node that we're changing. That would mean that when moving a group of selected nodes, the viewport wouldn't respond to them moving. Maybe a signal could be emitted whenever there was a need to re-calculate the node coordinates which would call something like updateSelectedStars

@cardinot
I updated it to utilize signals but I was wondering whether this is memory-safe or I need to explicitly delete the replaced star (or change its attributes in-place)

cardinot · 2019-08-27T21:42:31Z

src/gui/basegraphgl.cpp

@@ -147,32 +149,34 @@ void BaseGraphGL::slotDeleteSelectedNodes()
    updateCache();
 }

-void BaseGraphGL::createNode(QPointF pos)
+void BaseGraphGL::createNode(const QPointF pos)


pos should be passed as a const reference, i.e., const QPointF&
(the same for all other calls)

cardinot · 2019-08-27T22:04:40Z

src/gui/graphview.cpp

+                selectedStar.edges.push_back({ ep.second, line });
+            }
+        }
+        selectedStar.edges.shrink_to_fit();


this approach isn't very efficient either
because you'll always end up looping through all nodes twice

actually, this code is mostly a copy&paste from createStar(). Could we just call the createStart() function straight away when it's needed? (i.e., instead of emitting the nodesMoved() signals)

Could we just call the createStart() function straight away when it's needed? (i.e., instead of emitting the nodesMoved() signals)

Hm not sure I quite get that. Do you mean to refactor this without using signals?

yes, there's no gain in using the signal&slot in this case

My main point here is that your "slotUpdateSelection" is just a copy&paste from "createStar"; we don't need to duplicate this code.

I reformatted a bit as to directly call createStar, but I still can't see a more efficient way to call the UpdateSelection. This can't be called directly from the painting methods and neither can it not be called each time a node is moved. What am I missing?

Sedictious added 7 commits August 25, 2019 00:00

Add node deletion

f1e846b

NodeEdit: Add nodes

97da493

NodeEdit: Remove nodes

f8ef335

NodeEdit: Allow to move nodes

d91157b

Fixes

356a111

Allow to remove a single node from the full inspector

07139ee

More fixes

28fe85f

Sedictious force-pushed the nodeInspector branch from 5f7fd43 to 28fe85f Compare August 25, 2019 17:42

cardinot reviewed Aug 25, 2019

View reviewed changes

Sedictious added 2 commits August 26, 2019 08:52

Selected stars are correctly updated

7f46c72

Point is now const in moveNodes

abf001f

cardinot reviewed Aug 27, 2019

View reviewed changes

Sedictious force-pushed the nodeInspector branch from f75f180 to abf001f Compare August 29, 2019 10:28

Sedictious added 3 commits August 29, 2019 11:28

Make findNode const

8e4adab

Refactor slotUpdateSelection

c503329

NodeEdit: Fix clearing selection

d66e976

cardinot merged commit 6e2f064 into evoplex:socis19 Aug 31, 2019

cardinot added this to In Progress in graph designer via automation Sep 7, 2019

cardinot moved this from In Progress to Done in graph designer Sep 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nodeEdit mode #46

Add nodeEdit mode #46

Sedictious commented Aug 25, 2019

cardinot Aug 25, 2019

Sedictious Aug 26, 2019

cardinot Aug 27, 2019

cardinot Aug 25, 2019

Sedictious Aug 26, 2019

cardinot Aug 27, 2019

cardinot Aug 25, 2019

cardinot Aug 25, 2019

cardinot Aug 25, 2019

cardinot Aug 25, 2019

cardinot Aug 25, 2019

Sedictious Aug 26, 2019

Sedictious Aug 26, 2019

cardinot Aug 27, 2019 •

edited

cardinot Aug 27, 2019

Sedictious Aug 29, 2019

cardinot Aug 29, 2019

cardinot Aug 29, 2019

Sedictious Aug 30, 2019

Add nodeEdit mode #46

Add nodeEdit mode #46

Conversation

Sedictious commented Aug 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardinot Aug 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardinot Aug 27, 2019 •

edited