New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mdal api break #16

Closed

PeterPetrik wants to merge 10 commits into master from mdal_api_break

Owner

PeterPetrik commented Nov 30, 2018

Description

Include a few sentences describing the overall goals for this PR (pull request). If applicable also add screenshots.

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 contain 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

PeterPetrik added 9 commits

November 23, 2018 15:20


          WIP: implement new mdal api

211ae48


          introduce MeshDataBlock

f24c760


          use mesh block in rendering

037fc2a


          calculate magnitude

63a5348


          use new mesh API

99cce97


          fix bug for memory layer

81770a4


          fix SIP, sip is unable to work with qvector<qgsmeshvertex>

e953702


          fix tests


          implement new MDAL min/max api

8e00e3c

PeterPetrik requested a review from wonder-sk

November 30, 2018 10:48

wonder-sk reviewed

View reviewed changes

src/core/mesh/qgsmeshdataprovider.cpp Outdated

+              QgsMeshDatasetValue QgsMeshDataBlock::value( int index ) const
+              {
+                assert( ActiveFlagInteger != mType );

Collaborator

wonder-sk Dec 3, 2018

having asserts in public api is maybe a bit brutal (e.g. a plugin developer tests the api and gets crashes)

src/core/mesh/qgsmeshdataprovider.cpp

+                    mDoubleBuffer[2 * index + 1]
+                  );
+                  return val;
+                }

Collaborator

wonder-sk Dec 3, 2018

maybe it would be better to have separate functions to get scalar/vector value, with the correct return type

Owner Author

PeterPetrik Dec 4, 2018

I would keep this refactoring when we will get rid of Value class in QGIS.

src/core/mesh/qgsmeshdataprovider.cpp Outdated

+              QgsMeshVertex QgsMesh::vertex( int index ) const
+              {
+                if ( index < vertices.size() )

Collaborator

wonder-sk Dec 3, 2018

also index >= 0

src/core/mesh/qgsmeshdataprovider.h Outdated

+                  /**
+                   * Returns internal buffer to the array
+                   * The buffer is already allocated
+                   * Use to retreive data from MDAL provider

Collaborator

wonder-sk Dec 3, 2018

add details about what data length / type to expect from the buffer.

"Use to retreive data from MDAL provider" -- so with memory mesh provider this does not work? Probably should say something like use this to write data to the block.

Maybe worth noting it is primarily meant as a way to write data to the block (rather than for reading).

src/core/mesh/qgsmeshdataprovider.h

@@ @@ -221,9 +329,15 @@ class CORE_EXPORT QgsMeshDatasetMetadata @@
                    */
                   bool isValid() const;
+                  double minimum() const;
+                  double maximum() const;

Collaborator

wonder-sk Dec 3, 2018

missing docstrings?

src/core/mesh/qgsmeshdataprovider.h

+                  /**
+                   * \brief Returns N vector/scalar values from the index from the dataset
+                   *
+                   * See QgsMeshDatasetMetadata::isVector() to check if the returned value is vector or scalar

Collaborator

wonder-sk Dec 3, 2018

this information is also available in QgsMeshDataBlock

src/core/mesh/qgsmeshdataprovider.h Outdated

+                   * The buffer is already allocated
+                   * Use to retreive data from MDAL provider
+                   */
+                  void *buffer();

Collaborator

wonder-sk Dec 3, 2018

I would add SIP_SKIP here - I guess we don't want plugins potentially using this

src/core/mesh/qgsmeshdataprovider.h Outdated

+                   * The buffer is already allocated
+                   * Use to retreive data from MDAL provider
+                   */
+                  void *buffer();

Collaborator

wonder-sk Dec 3, 2018

Shall we also add const void *constBuffer() const; for efficient reading of data out of the block? (it would use constData() call on QVector instead of data() to avoid detach)

src/core/mesh/qgsmeshdataprovider.h

+                   *
+                   * See QgsMeshDatasetMetadata::isVector() to check if the returned value is vector or scalar
+                   */
+                  virtual QgsMeshDataBlock datasetValues( QgsMeshDatasetIndex index, int valueIndex, int count ) const = 0;

Collaborator

wonder-sk Dec 3, 2018

\since QGIS 3.6

src/core/mesh/qgsmeshdataprovider.h

                    *  V3 ---- V4 ---- V6-----V8
                    */
                   virtual bool isFaceActive( QgsMeshDatasetIndex index, int faceIndex ) const = 0;
+                  /**
+                   * \brief Returns whether the faces are active for particular dataset

Collaborator

wonder-sk Dec 3, 2018

\since QGIS 3.6


          improve mesh documentation

27411bd

PeterPetrik closed this

PeterPetrik pushed a commit that referenced this pull request


          Don't store references to symbols in 3d symbol handlers

a27930c

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet