Mesh gui #13

PeterPetrik · 2018-06-19T08:27:59Z

Missing bits (for next pull requests):

use raster shader widget for raster layer
add support for dataset groups to MDAL directly (now workaround in the dataset tree view widget)
write/read renderer setting from a project
persist datasets in a project
store renderer settings for groups, not for a whole layer
fix some bugs

wonder-sk

Do we need all the new widget classes in GUI library? If not, it would be better to move them to src/app for more flexibility...

wonder-sk · 2018-06-19T08:36:29Z

src/app/mesh/qgsmeshlayerproperties.cpp

+void QgsMeshLayerProperties::onCancel()
+{
+
+}


wonder-sk · 2018-06-19T08:37:48Z

src/app/mesh/qgsmeshlayerproperties.cpp

+
+QgsMeshLayerProperties::QgsMeshLayerProperties( QgsMapLayer *lyr, QgsMapCanvas *canvas, QWidget *parent, Qt::WindowFlags fl )
+  : QgsOptionsDialogBase( QStringLiteral( "MeshLayerProperties" ), parent, fl )
+  , mMeshLayer( qobject_cast<QgsMeshLayer *>( lyr ) )


how about we only expect QgsMeshLayer in constructor? should be safer and we would not need to test "if ( mMeshLayer )" in various places

if ( mMeshLayer ) is in the code because of pointer in the constuctor, not because of type cast to mesh layer. Do we want to replace it with reference?

Until now probably everywhere in qgis code we use pointers to map layers, so i would keep that for consistency. Maybe just add Q_ASSERT( mMeshLayer ) to the constructor?

wonder-sk · 2018-06-19T08:38:40Z

src/app/mesh/qgsmeshlayerproperties.cpp

+void QgsMeshLayerProperties::showHelp()
+{
+  QgsHelp::openHelp( QStringLiteral( "working_with_mesh/mesh_properties.html" ) );
+}


Better not to implement this (yet) if the page does not exist

wonder-sk · 2018-06-19T08:39:27Z

src/app/mesh/qgsmeshlayerproperties.h

+#include "qgsguiutils.h"
+// #include "qgshelp.h"
+// #include "qgsmaplayerstylemanager.h"
+// #include "qgsmaptoolemitpoint.h"


wonder-sk · 2018-06-19T08:40:04Z

src/app/mesh/qgsmeshlayerproperties.h

+ * Property sheet for a mesh map layer.
+ * Contains information, source and style tabs
+ *
+ * \since QGIS 3.4


not necessary to add \since because we are in src/app

wonder-sk · 2018-06-19T08:52:13Z

src/core/raster/qgscolorrampshader.cpp

+    mClassificationMode );
+  s->setColorRampItemList( colorRampItemList() ); //TODO?? why it is not copied in operator=?
+  return s;
+}


It looks like a bug/mistake that the color ramp item list is not copied in copy constructor / operator=. We should fix it and get rid of clone() otherwise it would get messy...

wonder-sk · 2018-06-19T08:54:58Z

src/gui/mesh/qgsmeshdatasetgrouptree.h

+ *
+ * \since QGIS 3.4
+ */
+class GUI_EXPORT QgsMeshDatasetGroupTree : public QTreeWidget


I would suggest to use QTreeView with QStandardItemModel instead of using QTreeWidget - it is more flexible and we ended up changing a QTreeWidget to QTreeView various times already...

Also it is good to add "View" (or "Widget") suffix so it is easy to see from the name it is derived from QWidget

wonder-sk · 2018-06-19T08:55:40Z

src/gui/mesh/qgsmeshdatasetgrouptree.h

+ *
+ * One dataset group is selected (active)
+ *
+ * \since QGIS 3.4


Shall we add a note about unstable API - or we will make it stable from QGIS 3.4?

wonder-sk · 2018-06-19T08:57:39Z

src/gui/mesh/qgsmeshdatasetgrouptree.h

+
+    QgsMeshLayer *mMeshLayer = nullptr; // not owned
+    QMap<QString, QVector<int>> mGroups; // group name -> dataset indices
+    QString mActiveGroup = QString();


not needed to do = QString() - that's the default

wonder-sk · 2018-06-19T09:05:37Z

src/gui/raster/qgscolorrampshaderwidget.cpp

+  // connect( mMinLineEdit, &QLineEdit::textChanged, this, &QgsColorRampShaderWidget::mMinLineEdit_textChanged );
+  // connect( mMaxLineEdit, &QLineEdit::textChanged, this, &QgsColorRampShaderWidget::mMaxLineEdit_textChanged );
+  // connect( mMinLineEdit, &QLineEdit::textEdited, this, &QgsColorRampShaderWidget::mMinLineEdit_textEdited );
+  // connect( mMaxLineEdit, &QLineEdit::textEdited, this, &QgsColorRampShaderWidget::mMaxLineEdit_textEdited );


quite a lot of commented out code in this file - i guess work in progress?

wonder-sk · 2018-06-22T13:09:46Z

src/core/mesh/qgsmeshdataprovider.h

@@ -215,6 +215,8 @@ class CORE_EXPORT QgsMeshDatasetSourceInterface SIP_ABSTRACT

    /**
     * \brief Associate dataset with the mesh
+     *
+     * emits dataChanged when successfull


wonder-sk · 2018-06-22T13:11:03Z

src/core/mesh/qgsmeshrenderersettings.h

+    //! Returns color ramp shader function
+    QgsColorRampShader colorRampShader() const;
+    //! Sets color ramp shader function
+    void setColorRampShader( QgsColorRampShader shader );


const QgsColorRampShader &

wonder-sk · 2018-06-22T13:13:18Z

src/gui/raster/qgscolorrampshaderwidget.h

+    void setMinMax( double min, double max );
+
+    //! Returns shared function used in the renderer. Caller takes ownership and deletes it.
+    QgsColorRampShader shader() const;


The comment above looks wrong - it returns object by value

wonder-sk · 2018-06-22T13:13:36Z

src/gui/raster/qgssinglebandpseudocolorrendererwidget.h

@@ -42,10 +42,16 @@ class GUI_EXPORT QgsSingleBandPseudoColorRendererWidget: public QgsRasterRendere

    static QgsRasterRendererWidget *create( QgsRasterLayer *layer, const QgsRectangle &extent ) SIP_FACTORY { return new QgsSingleBandPseudoColorRendererWidget( layer, extent ); }
    QgsRasterRenderer *renderer() override;
+
+    //! Returns shared function used in the renderer. Caller takes ownership and deletes it.


s/shared/shader/

wonder-sk · 2018-06-22T13:14:20Z

src/gui/raster/qgssinglebandpseudocolorrendererwidget.h

@@ -42,10 +42,16 @@ class GUI_EXPORT QgsSingleBandPseudoColorRendererWidget: public QgsRasterRendere

    static QgsRasterRendererWidget *create( QgsRasterLayer *layer, const QgsRectangle &extent ) SIP_FACTORY { return new QgsSingleBandPseudoColorRendererWidget( layer, extent ); }
    QgsRasterRenderer *renderer() override;
+
+    //! Returns shared function used in the renderer. Caller takes ownership and deletes it.
+    QgsColorRampShader *shaderFunction() const;


should have SIP_FACTORY annotation

wonder-sk · 2018-06-22T13:59:41Z

src/core/mesh/qgsmeshlayerrenderer.cpp

+    return; // no need for default shader, either rendering is off or we already have some shader
+
+  double vMin = *std::min_element( mScalarDatasetValues.constBegin(), mScalarDatasetValues.constEnd() );
+  double vMax = *std::max_element( mScalarDatasetValues.constBegin(), mScalarDatasetValues.constEnd() );


use minmax_element ?

wonder-sk · 2018-06-22T14:01:12Z

src/core/raster/qgscolorrampshader.cpp

 }

 QgsColorRampShader &QgsColorRampShader::operator=( const QgsColorRampShader &other )
 {
-  mSourceColorRamp.reset( other.sourceColorRamp()->clone() );
+  if ( other.sourceColorRamp() )
+    mSourceColorRamp.reset( other.sourceColorRamp()->clone() );


mSourceColorRamp must be set to nullptr if other.sourceColorRamp() is nullptr

wonder-sk · 2018-06-22T14:02:01Z

src/gui/raster/qgscolorrampshaderwidget.cpp

+                         ----------------------------
+    begin                : Jun 2018 by Peter Petrik
+    copyright            : (C) 2012 by Marco Hugentobler
+    email                : marco at sourcepole dot ch


this header looks weird

wonder-sk · 2018-06-22T14:05:59Z

src/gui/raster/qgscolorrampshaderwidget.cpp

+
+  QgsColorRampShader *colorRampShader = new QgsColorRampShader(
+    mMin, mMax,
+    ramp.get(),


ramp.release() because it takes ownership of the color ramp

use unique_ptr for colorRampShader

wonder-sk · 2018-06-22T14:13:07Z

src/core/mesh/qgsmeshlayerrenderer.cpp

+
+  QList<QgsColorRampShader::ColorRampItem> lst;
+  lst << QgsColorRampShader::ColorRampItem( vMin, Qt::blue, QString::number( vMin ) );
+  lst << QgsColorRampShader::ColorRampItem( vMax, Qt::red, QString::number( vMax ) );


shall we use the color ramp from settings?

QgsSettings settings; QString defaultPalette = settings.value( QStringLiteral( "/Raster/defaultPalette" ), "Spectral" ).toString(); btnColorRamp->setColorRampFromName( defaultPalette );

The blue-red color ramp is quite ugly as a default :-)

These references are to objects belonging to another thread, which can cause crashes if the object on the main thread is deleted while the handler is still active Refs a asan report: ==677416==ERROR: AddressSanitizer: heap-use-after-free on address 0x6070029bc278 at pc 0x7f95719ccc45 bp 0x7f94bdd7a310 sp 0x7f94bdd7a300 READ of size 4 at 0x6070029bc278 thread T36 (Thread (pooled)) #0 0x7f95719ccc44 in QgsLine3DSymbol::extrusionHeight() const /home/nyall/dev/qgis-asan/src/3d/symbols/qgsline3dsymbol.h:78 #1 0x7f95674a7c01 in QgsBufferedLine3DSymbolHandler::processFeature(QgsFeature&, Qgs3DRenderContext const&) /home/nyall/dev/qgis-asan/src/3d/symbols/qgsline3dsymbol_p.cpp:126 #2 0x7f9567457045 in operator() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:97 #3 0x7f95674597fd in runFunctor /usr/include/qt5/QtConcurrent/qtconcurrentstoredfunctioncall.h:70 #4 0x7f956744acf7 in QtConcurrent::RunFunctionTask<void>::run() (/home/nyall/dev/build-QGIS-asan/output/lib/libqgis_3d.so.3.15.0+0x2adcf7) #5 0x7f956a275d59 (/usr/lib64/libQt5Core.so.5+0xc9d59) #6 0x7f956a27268f (/usr/lib64/libQt5Core.so.5+0xc668f) #7 0x7f956a193431 in start_thread (/usr/lib64/libpthread.so.0+0x9431) #8 0x7f955ad59912 in __GI___clone (/usr/lib64/libc.so.6+0x101912) 0x6070029bc278 is located 56 bytes inside of 72-byte region [0x6070029bc240,0x6070029bc288) freed by thread T0 here: #0 0x7f9572b86b87 in operator delete(void*) (/usr/lib64/libasan.so.6+0xb2b87) #1 0x7f956749c00b in QgsLine3DSymbol::~QgsLine3DSymbol() /home/nyall/dev/qgis-asan/src/3d/symbols/qgsline3dsymbol.cpp:30 #2 0x7f95719664ca in std::default_delete<QgsAbstract3DSymbol>::operator()(QgsAbstract3DSymbol*) const /usr/include/c++/10/bits/unique_ptr.h:85 #3 0x7f95674428ce in std::unique_ptr<QgsAbstract3DSymbol, std::default_delete<QgsAbstract3DSymbol> >::~unique_ptr() /usr/include/c++/10/bits/unique_ptr.h:361 #4 0x7f9567459eb6 in QgsVectorLayerChunkLoaderFactory::~QgsVectorLayerChunkLoaderFactory() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.h:53 #5 0x7f9567459edd in QgsVectorLayerChunkLoaderFactory::~QgsVectorLayerChunkLoaderFactory() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.h:53 #6 0x7f956746e4db in QgsChunkedEntity::~QgsChunkedEntity() /home/nyall/dev/qgis-asan/src/3d/chunks/qgschunkedentity_p.cpp:122 #7 0x7f9567459358 in QgsVectorLayerChunkedEntity::~QgsVectorLayerChunkedEntity() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:164 #8 0x7f9567459373 in QgsVectorLayerChunkedEntity::~QgsVectorLayerChunkedEntity() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:168 #9 0x7f956a42e960 in QObject::event(QEvent*) (/usr/lib64/libQt5Core.so.5+0x282960) #10 0x7f956ada2062 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/lib64/libQt5Widgets.so.5+0x172062) previously allocated by thread T0 here: #0 0x7f9572b86067 in operator new(unsigned long) (/usr/lib64/libasan.so.6+0xb2067) #1 0x7f95674a3018 in qgis::_Unique_if<QgsLine3DSymbol>::_Single_object qgis::make_unique<QgsLine3DSymbol>() /home/nyall/dev/qgis-asan/src/core/qgis.h:425 #2 0x7f956749c0b9 in QgsLine3DSymbol::clone() const /home/nyall/dev/qgis-asan/src/3d/symbols/qgsline3dsymbol.cpp:34 #3 0x7f9567458b5f in QgsVectorLayerChunkLoaderFactory::QgsVectorLayerChunkLoaderFactory(Qgs3DMapSettings const&, QgsVectorLayer*, QgsAbstract3DSymbol*, int) /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:141 #4 0x7f9567458d88 in QgsVectorLayerChunkedEntity::QgsVectorLayerChunkedEntity(QgsVectorLayer*, double, double, QgsVectorLayer3DTilingSettings const&, QgsAbstract3DSymbol*, Qgs3DMapSettings const&) /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:159 #5 0x7f956745558f in QgsVectorLayer3DRenderer::createEntity(Qgs3DMapSettings const&) const /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayer3drenderer.cpp:76 #6 0x7f95673a71c0 in Qgs3DMapScene::addLayerEntity(QgsMapLayer*) /home/nyall/dev/qgis-asan/src/3d/qgs3dmapscene.cpp:693 #7 0x7f95673a5523 in Qgs3DMapScene::onLayerRenderer3DChanged() /home/nyall/dev/qgis-asan/src/3d/qgs3dmapscene.cpp:598 #8 0x7f95673c6c33 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (Qgs3DMapScene::*)()>::call(void (Qgs3DMapScene::*)(), Qgs3DMapScene*, void**) (/home/nyall/dev/build-QGIS-asan/output/lib/libqgis_3d.so.3.15.0+0x229c33) #9 0x7f95673c5468 in void QtPrivate::FunctionPointer<void (Qgs3DMapScene::*)()>::call<QtPrivate::List<>, void>(void (Qgs3DMapScene::*)(), Qgs3DMapScene*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:185 #10 0x7f95673c1748 in QtPrivate::QSlotObject<void (Qgs3DMapScene::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt5/QtCore/qobjectdefs_impl.h:418 #11 0x7f956a435f75 (/usr/lib64/libQt5Core.so.5+0x289f75) #12 0x7f956252106c in QgsMapLayer::renderer3DChanged() src/core/qgis_core_autogen/EWIEGA46WW/moc_qgsmaplayer.cpp:702 #13 0x7f9563917a12 in QgsMapLayer::setRenderer3D(QgsAbstract3DRenderer*) /home/nyall/dev/qgis-asan/src/core/qgsmaplayer.cpp:1813 #14 0x7f95719aaf52 in QgsApp3DSymbolWidgetWithPreview::updatePreview(QgsAbstract3DSymbol*) /home/nyall/dev/qgis-asan/src/app/3d/qgsapp3dsymbolwidget.cpp:243 #15 0x7f95719a97af in operator() /home/nyall/dev/qgis-asan/src/app/3d/qgsapp3dsymbolwidget.cpp:175 #16 0x7f95719ac2dd in call /usr/include/qt5/QtCore/qobjectdefs_impl.h:146 #17 0x7f95719ac24f in call<QtPrivate::List<>, void> /usr/include/qt5/QtCore/qobjectdefs_impl.h:256 #18 0x7f95719ac21e in impl /usr/include/qt5/QtCore/qobjectdefs_impl.h:443 #19 0x7f956a435f75 (/usr/lib64/libQt5Core.so.5+0x289f75) #20 0x7f95708d15ec in QgsApp3DSymbolWidget::widgetChanged() src/app/qgis_app_autogen/6LADBHSVD5/moc_qgsapp3dsymbolwidget.cpp:144 #21 0x7f95719ae1dd in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (QgsApp3DSymbolWidget::*)()>::call(void (QgsApp3DSymbolWidget::*)(), QgsApp3DSymbolWidget*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:152 #22 0x7f95719add9c in void QtPrivate::FunctionPointer<void (QgsApp3DSymbolWidget::*)()>::call<QtPrivate::List<>, void>(void (QgsApp3DSymbolWidget::*)(), QgsApp3DSymbolWidget*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:185 #23 0x7f95719adbfa in QtPrivate::QSlotObject<void (QgsApp3DSymbolWidget::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt5/QtCore/qobjectdefs_impl.h:418 #24 0x7f956a435f75 (/usr/lib64/libQt5Core.so.5+0x289f75) #25 0x7f956c91c63e in Qgs3DSymbolWidget::changed() src/gui/qgis_gui_autogen/EWIEGA46WW/moc_qgs3dsymbolwidget.cpp:131 #26 0x7f95719ce2ad in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (Qgs3DSymbolWidget::*)()>::call(void (Qgs3DSymbolWidget::*)(), Qgs3DSymbolWidget*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:152 #27 0x7f95719ce014 in void QtPrivate::FunctionPointer<void (Qgs3DSymbolWidget::*)()>::call<QtPrivate::List<>, void>(void (Qgs3DSymbolWidget::*)(), Qgs3DSymbolWidget*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:185 #28 0x7f95719cdbfc in QtPrivate::QSlotObject<void (Qgs3DSymbolWidget::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt5/QtCore/qobjectdefs_impl.h:418 #29 0x7f956a435f75 (/usr/lib64/libQt5Core.so.5+0x289f75)

PeterPetrik added 6 commits June 15, 2018 09:20

[mesh] Style contours/scalars in GUI

104e059

[mesh] Select active dataset in GUI

0b17970

[mesh] Style mesh frame in GUI

24d9e7b

[mesh] Style vector arrows in GUI

709cb51

[mesh] basic mesh layer properties dialog

b960ba0

add docs and cleanup includes

a4bf276

PeterPetrik assigned wonder-sk Jun 19, 2018

wonder-sk reviewed Jun 19, 2018

View reviewed changes

PeterPetrik added 5 commits June 20, 2018 07:24

move mesh files to app

1a40476

various code cleaning

ff670e0

replace treewidget with treeview

17e2434

remember last used folder for adding datasets

ec52aff

update mesh renderer widgets when datasets are added

eebf9a1

wonder-sk reviewed Jun 22, 2018

View reviewed changes

PeterPetrik unassigned wonder-sk Jun 25, 2018

PeterPetrik added 2 commits June 25, 2018 17:34

fix issues from review

cd78fbd

fix coredump when opening properties panel

c94865d

PeterPetrik closed this Jun 26, 2018

PeterPetrik deleted the mesh_gui branch June 29, 2018 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mesh gui #13

Mesh gui #13

PeterPetrik commented Jun 19, 2018 •

edited

Loading

wonder-sk left a comment

wonder-sk Jun 19, 2018

wonder-sk Jun 19, 2018

PeterPetrik Jun 20, 2018

wonder-sk Jun 20, 2018

wonder-sk Jun 19, 2018

wonder-sk Jun 19, 2018

PeterPetrik Jun 20, 2018

wonder-sk Jun 19, 2018

wonder-sk Jun 19, 2018

wonder-sk Jun 19, 2018

wonder-sk Jun 19, 2018

wonder-sk Jun 19, 2018

wonder-sk Jun 19, 2018

wonder-sk Jun 19, 2018

wonder-sk Jun 22, 2018

wonder-sk Jun 22, 2018

wonder-sk Jun 22, 2018

wonder-sk Jun 22, 2018

wonder-sk Jun 22, 2018

wonder-sk Jun 22, 2018

wonder-sk Jun 22, 2018

wonder-sk Jun 22, 2018

wonder-sk Jun 22, 2018

wonder-sk Jun 22, 2018

Mesh gui #13

Mesh gui #13

Conversation

PeterPetrik commented Jun 19, 2018 • edited Loading

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

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

PeterPetrik commented Jun 19, 2018 •

edited

Loading