[BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features. Second version #6750

lbartoletti · 2018-04-05T06:50:38Z

Description

Following the discussions, I open a new PR with modifications for #6657 / Fixes #16838.

If you are ok with the new propositions, I will add tests

A comment, I thought that by activating/deactivating a style, the styleChanged or rendererChanged signal would be emitted. This is not the case, I added the repaintRequested signal. See the video

I tested on a data set with the cadastre and all networks (water, sanitation, road, etc.) on an old computer and for me it is usable.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

Commit messages are descriptive and explain the rationale for changes
Commits which fix bugs include fixes #11111 in the commit message next to the description
Commits which add new features are tagged with [FEATURE] in the commit message
Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
I have read the QGIS Coding Standards and this PR complies with them
This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
New unit tests have been added for core changes
I have run the scripts/prepare-commit.sh script before each commit

nyalldawson

Looking much better - this is close now!

Don't forget to add unit tests too.

nyalldawson · 2018-04-06T00:25:21Z

src/core/qgspointlocator.cpp

+  connect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex );
+  connect( mLayer, &QgsVectorLayer::layerModified, this, &QgsPointLocator::destroyIndex );
+  // used when a style is (de)activated
+  connect( mLayer, &QgsVectorLayer::repaintRequested, this, &QgsPointLocator::destroyIndex );


Not sure about destroying the index on a repaint and layer modified - these signals are thrown whenever a feature is edited in a layer - so by destroying the index when that occurs it's forcing a full rebuild, instead of the faster update handling done by onFeatureDeleted/onGeometryChanged. (I'd also suggest working on a test data set with 100k+ features while test running this.. that'll be bound to reveal any performance regressions!)

I think you'll need to fix the missing styleChanged signal instead.

nyalldawson · 2018-04-06T00:28:29Z

src/core/qgspointlocator.cpp

@@ -685,7 +694,8 @@ bool QgsPointLocator::rebuildIndex( int maxFeaturesToIndex )
    return true; // nothing to index

  QgsFeatureRequest request;
-  request.setSubsetOfAttributes( QgsAttributeList() );
+  //request.setSubsetOfAttributes( QgsAttributeList() );


Fetching attributes here isn't required, is it? I think this is leftover from an earlier iteration and will cause a performance drop.

I add this request in an else

nyalldawson · 2018-04-06T00:30:36Z

src/core/qgspointlocator.cpp

@@ -685,7 +694,8 @@ bool QgsPointLocator::rebuildIndex( int maxFeaturesToIndex )
    return true; // nothing to index

  QgsFeatureRequest request;
-  request.setSubsetOfAttributes( QgsAttributeList() );
+  //request.setSubsetOfAttributes( QgsAttributeList() );
+  request.setFilterFids( mFeatureIds );


This should be:

if ( !mFeatureIds.empty() ) request.setFilterFids( mFeatureIds );

(I think ideally QgsFeatureRequest should ignore empty feature lists, but at the moment it still sets the filter type to FilterFids even if an empty list is given -- that would need careful testing though, so go with the easy option here).

done with an else for request.setSubsetOfAttributes

nyalldawson · 2018-04-06T00:31:15Z

src/core/qgssnappingutils.cpp

@@ -330,6 +330,57 @@ QgsPointLocator::Match QgsSnappingUtils::snapToMap( const QgsPointXY &pointMap,
 }


+QgsFeatureIds QgsSnappingUtils::isMatchAVisibleLayer( QgsVectorLayer *layer )


rename to findVisibleFeatures

nyalldawson · 2018-04-06T00:32:45Z

src/core/qgssnappingutils.cpp

+        renderer->stopRender( context );
+      }
+        }
+        else {


I don't think this else branch is needed - if the invisible skipping is disabled, you could return an empty list here. That'll be handled correctly by the request in QgsPointLocator::rebuildIndex and avoid the expensive full layer iteration performed here.

nyalldawson · 2018-04-06T00:34:13Z

src/core/qgssnappingutils.cpp

@@ -359,7 +411,13 @@ void QgsSnappingUtils::prepareIndex( const QList<LayerAndAreaOfInterest> &layers
      QgsVectorLayer *vl = entry.first;
      QTime tt;
      tt.start();
+
+      QgsFeatureIds fids = isMatchAVisibleLayer( vl );


I think here you may need to check:

if ( mEnableSnappingForInvisibleFeature && fids.empty() )
continue; //no visible features, skip layer

done with a minor change

if ( !mEnableSnappingForInvisibleFeature && fids.empty() ) continue; //no visible features, skip layer

nyalldawson · 2018-04-06T00:38:05Z

src/core/qgssnappingutils.cpp

+      }
+
+
+      QgsFeatureIterator fi = layer->getFeatures();


Use an optimised request here:

use the renderer filter (as explored earlier) to partially limit the requested features

set the subset of attributes to match only those required by the renderer

maybe limit to mapSettings extent? (not sure)

I don't think we should limit to map extent.
ping @wonder-sk

The decision whether to limit to mapSettings extent should depend on the indexing strategy of the locator - if the locator indexes only a particular extent, the request should use it, but if the locator indexes everything, no extent should be set.

Looking at the current approach, one thing that worries me is that now we need to iterate over the vector layer twice: first to check whether the features are actually visible and store the fids, second time to build the actual locator index. Another problem is that most of the time all (or nearly all) features will be visible, and constructing feature requests with large number of filtered FIDs is going to be inefficient - for example, for postgres that means a generating and processing a query like this: "SELECT .... WHERE fid IN (1, 2, 3, 4, ..., 10000)"

How about updating QgsPointLocator::init() method to have optional check of feature visibility based on a passed feature renderer?

@3nids @wonder-sk @nyalldawson
Thank you for your reviews.
With my last commit, I have good performance for normal case (at least my use case : cadastre, road and networks, land survey, etc.). It hangs only on very big layer (more than 100k) when is filtered.

I wonder, if mapExtent should be the solution for locators created from QgsMapCanvasSnappingUtils?

@wonder-sk

How about updating QgsPointLocator::init() method to have optional check of feature visibility >based on a passed feature renderer?

Can you explain more please, I'm not sure if I understand? Is mEnableSnappingForInvisibleFeature flag not do this?

So right now in the first phase you build a set of feature IDs that will be used, then in the second phase you pass it to QgsPointLocator which uses the feature IDs in the rebuildIndex() method. What I am suggesting is to merge these two phases into one: rebuildIndex() would have access to a renderer (if enableSnappingForInvisibleFeature=false) and check its willRenderFeature() and add only visible features to the index. In this way we would avoid creation of the temporary set of feature IDs and we would not need to iterate over the layers twice.

@wonder-sk OK I Understand. It was my first idea but I haven't been able to fix all incomplete type and circular include. Maybe I need to take a closer look.

3nids · 2018-04-06T11:39:35Z

src/core/qgssnappingutils.h

+     *
+     * \since QGIS 3.2
+     */
+    void setEnableSnappingForInvisibleFeature( bool enableIt );


s/enableIt/enable/

Done. Sorry it was forgotten from precedent version.

3nids · 2018-04-06T11:44:17Z

src/core/qgssnappingutils.cpp

+
+  if ( layer )
+  {
+    if ( !mEnableSnappingForInvisibleFeature )


I think this should not be checked here but rather before the call at line 407 in prepareIndex().

lbartoletti · 2018-04-11T05:18:39Z

I will add tests later

lbartoletti · 2018-04-19T06:27:45Z

If it's OK, can you merge it please ?

nyalldawson · 2018-04-19T06:34:29Z

From a quick glance there's still some outstanding comments which haven't been addressed yet - e.g. not rebuilding the index on a triggerRepaint, and @wonder-sk's request to avoid the setFilterFids call

lbartoletti · 2018-04-21T11:32:56Z

Now, it's done :)

nyalldawson · 2018-04-21T11:59:59Z

src/core/qgspointlocator.cpp

@@ -636,6 +637,9 @@ QgsPointLocator::QgsPointLocator( QgsVectorLayer *layer, const QgsCoordinateRefe
  connect( mLayer, &QgsVectorLayer::featureDeleted, this, &QgsPointLocator::onFeatureDeleted );
  connect( mLayer, &QgsVectorLayer::geometryChanged, this, &QgsPointLocator::onGeometryChanged );
  connect( mLayer, &QgsVectorLayer::dataChanged, this, &QgsPointLocator::destroyIndex );
+  connect( mLayer, &QgsVectorLayer::rendererChanged, this, &QgsPointLocator::destroyIndex );
+  connect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex );
+  connect( mLayer, &QgsVectorLayer::layerModified, this, &QgsPointLocator::destroyIndex );


@wonder-sk can you think of a way to avoid this? My interpretation is that it will force a full rebuild when a feature is changed (since it could change the renderer result). But looking above, it seems we rebuild the index on a dataChanged already anyway...

Yes, unfortunately, but it's necessary.

I have a feeling this may be a blocker: this effectively means that even with every single change of geometry we would be destroying the index - that's going to make digitizing slow... Currently if there is e.g. 1-2 seconds initial lag when the index if first built, such lag would happen after every single move of a node when editing geometries...

Why is it necessary to destroy the index? I would expect we just need to modify existing handlers of featureAdded, featureDeleted, geometryChanged signal handlers to accommodate renderer-dependent search.

nyalldawson · 2018-04-21T12:01:32Z

src/core/qgspointlocator.cpp

@@ -685,7 +695,8 @@ bool QgsPointLocator::rebuildIndex( int maxFeaturesToIndex )
    return true; // nothing to index

  QgsFeatureRequest request;
-  request.setSubsetOfAttributes( QgsAttributeList() );
+  // request.setSubsetOfAttributes( QgsAttributeList() );


I'd leave this, and then in the branch below when a renderer is set you should set the subset of attributes to that required by the renderer.

nyalldawson · 2018-04-21T12:02:54Z

src/core/qgspointlocator.cpp

+
+  bool filter = false;
+  std::unique_ptr< QgsFeatureRenderer > renderer( mLayer->renderer() ? mLayer->renderer()->clone() : nullptr );
+  QgsRenderContext *ctx = nullptr;


Couldn't you just use mContext directly instead of ctx?

No sorry, it doesn't work :/

nyalldawson · 2018-04-21T12:04:36Z

src/core/qgspointlocator.cpp

+    {
+      // setup scale for scale dependent visibility (rule based)
+      renderer->startRender( *ctx, mLayer->fields() );
+      filter = renderer->capabilities() & QgsFeatureRenderer::Filter;


set request's filter expression here to renderer->filter()

Well, if I do request.setFilterExpression(renderer->filter()); with a request.setSubsetOfAttributes( QgsAttributeList() ); before. I can't snap on features when the style is RuleBasedRenderer and one of filter is unchecked. For a layer with a categorized renderer, if you uncheck a category, invisible feature can be snapped.

That's why in this branch you should set the subset of attributes to those required by the renderer (renderer->usedAttributes())

oh yes. Thank you!

nyalldawson · 2018-04-21T12:05:16Z

src/core/qgspointlocator.cpp

@@ -832,6 +875,8 @@ void QgsPointLocator::onFeatureDeleted( QgsFeatureId fid )
    mRTree->deleteData( rect2region( mGeoms[fid]->boundingBox() ), fid );
    delete mGeoms.take( fid );
  }
+
+  mFeatureIds.remove( fid );


I think this isn't needed - it's just a leftover from an earlier version

nyalldawson · 2018-04-21T12:06:17Z

src/core/qgssnappingutils.h

+     *
+     * \returns the QgsFeatureIds to index
+     */
+    QgsFeatureIds findVisibleFeatures( QgsVectorLayer *layer );


Remove leftover declaration

nyalldawson · 2018-04-21T12:07:45Z

Great work @lbartoletti - this iteration looks excellent

lbartoletti · 2018-04-24T06:58:22Z

Are you OK to merge it now ?

Thank you all.

wonder-sk

Good work @lbartoletti however I think we need to sort out the performance issue when layer gets modified before merging this...

wonder-sk · 2018-04-24T07:22:33Z

src/core/layertree/qgslayertreemodellegendnode.cpp

@@ -331,6 +331,7 @@ bool QgsSymbolLegendNode::setData( const QVariant &value, int role )
  vlayer->renderer()->checkLegendSymbolItem( mItem.ruleKey(), value == Qt::Checked );

  emit dataChanged();
+  emit vlayer->styleChanged();


Better to replace this with vlayer->emitStyleChanged() - we should not emit signals on behalf of other classes...

wonder-sk · 2018-04-24T07:26:08Z

src/core/qgspointlocator.cpp

@@ -636,6 +637,9 @@ QgsPointLocator::QgsPointLocator( QgsVectorLayer *layer, const QgsCoordinateRefe
  connect( mLayer, &QgsVectorLayer::featureDeleted, this, &QgsPointLocator::onFeatureDeleted );
  connect( mLayer, &QgsVectorLayer::geometryChanged, this, &QgsPointLocator::onGeometryChanged );
  connect( mLayer, &QgsVectorLayer::dataChanged, this, &QgsPointLocator::destroyIndex );
+  connect( mLayer, &QgsVectorLayer::rendererChanged, this, &QgsPointLocator::destroyIndex );
+  connect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex );


I think the index should get destroyed only when the point locator is dependent on visible features

wonder-sk · 2018-04-24T08:14:18Z

src/core/qgspointlocator.cpp

@@ -636,6 +637,9 @@ QgsPointLocator::QgsPointLocator( QgsVectorLayer *layer, const QgsCoordinateRefe
  connect( mLayer, &QgsVectorLayer::featureDeleted, this, &QgsPointLocator::onFeatureDeleted );
  connect( mLayer, &QgsVectorLayer::geometryChanged, this, &QgsPointLocator::onGeometryChanged );
  connect( mLayer, &QgsVectorLayer::dataChanged, this, &QgsPointLocator::destroyIndex );
+  connect( mLayer, &QgsVectorLayer::rendererChanged, this, &QgsPointLocator::destroyIndex );
+  connect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex );
+  connect( mLayer, &QgsVectorLayer::layerModified, this, &QgsPointLocator::destroyIndex );


I have a feeling this may be a blocker: this effectively means that even with every single change of geometry we would be destroying the index - that's going to make digitizing slow... Currently if there is e.g. 1-2 seconds initial lag when the index if first built, such lag would happen after every single move of a node when editing geometries...

Why is it necessary to destroy the index? I would expect we just need to modify existing handlers of featureAdded, featureDeleted, geometryChanged signal handlers to accommodate renderer-dependent search.

wonder-sk · 2018-04-24T08:15:49Z

src/core/qgspointlocator.cpp

+  mContext = std::unique_ptr<QgsRenderContext>( new QgsRenderContext( context ) );
+
+  destroyIndex();
+}


It seems to me there is no way to turn off visibility-dependent behavior of point locator after it has been enabled?

It is possible to configure the behavior in the preferences and then rebuild the index

Why is it necessary to destroy the index?

It's necessary to destroy the index, if you change the style of your layer, since the subject is especially to filter invisible feature.
Initially there was a bug when you checked a "sub style". I added possible cases.
In use, only styleChanged is really needed. What do you think if I do this:

remove:
connect( mLayer, &QgsVectorLayer::rendererChanged, this, &QgsPointLocator::destroyIndex ); connect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex ); connect( mLayer, &QgsVectorLayer::layerModified, this, &QgsPointLocator::destroyIndex );

add connect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex ); in QgsPointLocator::setRenderContext

wonder-sk · 2018-04-24T08:20:55Z

src/core/qgspointlocator.cpp

+    if ( ctx && renderer )
+    {
+      ctx->expressionContext().setFeature( f );
+      if ( filter && !renderer->willRenderFeature( f, *ctx ) )


It seems like the "filter" variable could be already checked in the "if" statement two lines above...

right. done.

wonder-sk · 2018-04-24T08:21:24Z

src/core/qgspointlocator.h

@@ -278,11 +285,15 @@ class CORE_EXPORT QgsPointLocator : public QObject
    //! flag whether the layer is currently empty (i.e. mRTree is null but it is not necessary to rebuild it)
    bool mIsEmptyLayer;

+    QgsFeatureIds mFeatureIds;


Not used anymore?

Yes a leftover...

wonder-sk · 2018-04-24T08:25:01Z

tests/src/core/testqgssnappingutils.cpp

+      QVERIFY( !m.hasVertex() );
+
+      u.setEnableSnappingForInvisibleFeature( true );
+      mVL->styleChanged();


It should not be necessary to emit styleChanged() here - the setEnableSnappingForInvisibleFeature() should take care of the invalidation of point locators.

wonder-sk · 2018-04-25T07:38:11Z

It is possible to configure the behavior in the preferences and then rebuild the index

If I have a QgsPointLocator instance with a QgsRenderContext assigned, there is no way to unassign it: I think the setRenderContext() method should have pointer to QgsRenderContext as an argument - if it is nullptr, the internal mContext would be also reset to nullptr.

Moving of connect() to setRenderContext() and avoiding layerModified() signal looks good, but I see two new problems:

every call to setRenderContext() will now add another connection to destroyIndex() - over time this can degrade performance because destroyIndex() maybe be called e.g. 1000 times for one signal
for a completely correct implementation, we should have support for visible features in onFeatureAdded, onGeometryChanged, and maybe add onAttributesChanged slot as well. If not added, invisible features may be added to the index. When assuming that only visible features will be added/removed/modified, then we could probably skip that for now, to keep the code simpler and finally get this PR merged...

… and onAttributeValueChanged

lbartoletti · 2018-04-25T15:29:36Z

@wonder-sk I have added support onFeatureAdded and onAttributeValueChanged.
For setRenderContext, i'm not sure about me ; if I understand your request.

wonder-sk

Looking great - nearly ready to merge.

Could you please add a test that check that a feature is registered (or not) when it is added to a layer, depending on whether it is visible or not?

wonder-sk · 2018-04-30T06:27:18Z

src/core/qgspointlocator.cpp

@@ -792,6 +842,30 @@ void QgsPointLocator::onFeatureAdded( QgsFeatureId fid )
    if ( !f.hasGeometry() )
      return;

+    std::unique_ptr< QgsFeatureRenderer > renderer( mLayer->renderer() ? mLayer->renderer()->clone() : nullptr );


It would be better to be cloned inside "if ( mContext )" block, so we do not copy it when snapping also to invisible features.

wonder-sk · 2018-04-30T06:29:42Z

src/core/qgspointlocator.cpp

+  Q_UNUSED( idx );
+  Q_UNUSED( value );
+  onFeatureDeleted( fid );
+  onFeatureAdded( fid );


These two functions should be called only when snapping to visible features...

lbartoletti · 2018-04-30T15:44:34Z

@wonder-sk is it ok for you? Thanks.

wonder-sk · 2018-05-01T07:46:25Z

Looking good!

lbartoletti · 2018-05-02T06:26:34Z

Thank you all.

Docs for this PR qgis/QGIS#6750

saberraz · 2018-05-03T08:31:02Z

@lbartoletti it should also not snap to layers not visible within that zoom when digitizing. Do you want me to file a bug?

saberraz · 2018-05-03T08:32:21Z

Also, features not available for snapping can be traced. Now we have inconsistent behaviours for snapping and tracing.

lbartoletti · 2018-05-03T10:29:27Z

@saberraz yes please, create this two issues. Thanks.

nyalldawson reviewed Apr 6, 2018

View reviewed changes

3nids reviewed Apr 6, 2018

View reviewed changes

lbartoletti closed this Apr 12, 2018

lbartoletti reopened this Apr 12, 2018

lbartoletti closed this Apr 13, 2018

lbartoletti reopened this Apr 13, 2018

lbartoletti changed the title ~~[WIP][BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features. Second version~~ [BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features. Second version Apr 17, 2018

lbartoletti closed this Apr 21, 2018

lbartoletti reopened this Apr 21, 2018

nyalldawson reviewed Apr 21, 2018

View reviewed changes

Fix snapping on invisible geometry

66b0e59

wonder-sk reviewed Apr 24, 2018

View reviewed changes

fix martin's review

5e5fdf9

lbartoletti closed this Apr 24, 2018

lbartoletti reopened this Apr 24, 2018

lbartoletti added 2 commits April 25, 2018 15:33

Add support for visible features in onFeatureAdded, onGeometryChanged…

43e905d

… and onAttributeValueChanged

try disconnect/connect for destroyIndex

07caafe

wonder-sk reviewed Apr 30, 2018

View reviewed changes

Improve onFeatureAdded and onAttributeValueChanged

68f463e

lbartoletti closed this Apr 30, 2018

lbartoletti reopened this Apr 30, 2018

clean tests and add a new test for snapping on inivisble feature

9f3d571

wonder-sk merged commit 3b36cdd into qgis:master May 1, 2018

lbartoletti deleted the visibleSnapPerf branch May 2, 2018 06:26

lbartoletti mentioned this pull request May 2, 2018

[QGIS] Snapping on invisible feature qwat/QWAT#193

Open

lbartoletti added a commit to lbartoletti/QGIS-Documentation that referenced this pull request May 2, 2018

Default snapping on visible features

293da68

Docs for this PR qgis/QGIS#6750

lbartoletti mentioned this pull request May 2, 2018

Default snapping on visible features qgis/QGIS-Documentation#2595

Merged

This was referenced May 25, 2019

snapping allows to snap non displayed legend classes #24737

Closed

Scale dependent visibility and snapping #26733

Open

Snapping and tracing inconsistent behaviour #26734

Closed

		@@ -330,6 +330,57 @@ QgsPointLocator::Match QgsSnappingUtils::snapToMap( const QgsPointXY &pointMap,
		}


		QgsFeatureIds QgsSnappingUtils::isMatchAVisibleLayer( QgsVectorLayer *layer )

[BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features. Second version #6750

[BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features. Second version #6750

Conversation

lbartoletti commented Apr 5, 2018 • edited

Description

Checklist

nyalldawson left a 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

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

lbartoletti commented Apr 11, 2018

lbartoletti commented Apr 19, 2018

nyalldawson commented Apr 19, 2018

lbartoletti commented Apr 21, 2018

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

Choose a reason for hiding this comment

nyalldawson commented Apr 21, 2018

lbartoletti commented Apr 24, 2018

wonder-sk left a 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

Choose a reason for hiding this comment

wonder-sk commented Apr 25, 2018

lbartoletti commented Apr 25, 2018

wonder-sk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbartoletti commented Apr 30, 2018

wonder-sk commented May 1, 2018

lbartoletti commented May 2, 2018

saberraz commented May 3, 2018

saberraz commented May 3, 2018

lbartoletti commented May 3, 2018

lbartoletti commented Apr 5, 2018 •

edited