Skip to content
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.

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

[FEATURE] QgsMeshLayer part 1: Reading raw mesh #6820

Merged
merged 6 commits into from
Apr 25, 2018

Conversation

PeterPetrik
Copy link
Contributor

Introducting MDAL, QgsMeshLayer, mesh data providers (mesh_memory, mdal)
to read and visualize raw meshes: vertices and faces. Support dragging
2dm files from browser on canvas to visualize 2dm meshes. Support for QgsMeshLayer in Python API.
See QEP Unstructured Mesh Layers

QgsMeshLayer

Introducting MDAL, QgsMeshLayer, mesh data providers (mesh_memory, mdal)
to read and visualize raw meshes: vertices and faces. Support dragging
2dm files from browser on canvas to visualize 2dm meshes.
Support for QgsMeshLayer in Python API.
@@ -1177,6 +1190,7 @@ INCLUDE_DIRECTORIES(
scalebar
symbology
metadata
mesh
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to add core/mesh to the doxygen paths too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

mProviderKey = provider;
QString dataSource = mDataSource;

if ( mDataProvider )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to project calls to delete - they do nothing for a nullptr anyway

//! Return const data provider
const QgsMeshDataProvider *dataProvider() const override SIP_SKIP;

/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't add docs for overridden methods unless they specifically add something relevant to the subclass - otherwise doxygen no longer inherits the base class docs and the subclass docs can go out of date and miss base class doxygen improvements.


private:
//! Pointer to native mesh structure, used as cache for rendering
QgsMesh *mNativeMesh = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use unique_ptr for these members (all except the QObject derived data provider)


// copy from mesh layer
QgsSymbol *mNativeMeshSymbol = nullptr;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unique ptrs



QgsTriangularMesh::QgsTriangularMesh( )
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=default

}

QgsTriangularMesh::~QgsTriangularMesh()
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= default (in header)

// CREATE TRIANGULAR MESH
for ( int i = 0; i < nativeMesh->faces.size(); ++i )
{
QgsMeshFace face = nativeMesh->faces[i] ;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may detach - use faces.at( i ) instead (and use Qt creator 4.6 with all levels of clazy warnings enabled to catch these)

else if ( type == QgsMapLayer::MeshLayer )
{
QgsDebugMsg( "creating mesh layer" );
QgsMeshLayer *layer = new QgsMeshLayer( layerItem->uri(), layerItem->uri(), layerItem->providerKey() );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unique_ptr

Copy link
Contributor Author

@PeterPetrik PeterPetrik Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather keep it this way, since both if ( type == QgsMapLayer::RasterLayer ) and else if ( type == QgsMapLayer::VectorLayer ) do it without unique_ptr

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boy Scout that code then! Don't let new code be influenced by existing bad practice, instead fix the existing code and leave the codebase in a cleaner state then it was before.

QgsTriangularMesh *triangularMesh() SIP_SKIP {return mTriangularMesh;}

//! Returns a line symbol used for rendering native mesh.
QgsSymbol *nativeMeshSymbol() {return mNativeMeshSymbol;}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about having symbol and toggleTriangularMeshRendering here - those seem to be more of mesh renderer properties, and I think belong in their own separate class.

@nyalldawson
Copy link
Collaborator

Cool! Exciting stuff

@PeterPetrik
Copy link
Contributor Author

@nyalldawson thank you for your review again, all should be OK now

@wonder-sk wonder-sk merged commit ade216d into qgis:master Apr 25, 2018
@m-kuhn
Copy link
Member

m-kuhn commented May 3, 2018

This triggers some build warnings here

[1552/3222] Building CXX object src/providers/mdal/CMakeFiles/mdalprovider.dir/qgsmdalprovider.cpp.o
In file included from src/providers/mdal/qgsmdalprovider.cpp:18:
src/providers/mdal/qgsmdalprovider.h:16:9: warning: 'QGSMDALPROVIDER_H' is used as a header guard here, followed by #define of a different macro [-Wheader-guard]
#ifndef QGSMDALPROVIDER_H
        ^~~~~~~~~~~~~~~~~
src/providers/mdal/qgsmdalprovider.h:17:9: note: 'QGSGDALPROVIDER_H' is defined here; did you mean 'QGSMDALPROVIDER_H'?
#define QGSGDALPROVIDER_H
        ^~~~~~~~~~~~~~~~~
        QGSMDALPROVIDER_H
1 warning generated.

PS: realized that I am working on an old codebase, might be solved meanwhile.

@PeterPetrik
Copy link
Contributor Author

PeterPetrik commented May 3, 2018

@m-kuhn I think it is already resolved

2d4b482

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants