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

[mesh] [feature] function to get value for the point on map #7582

Merged
merged 3 commits into from
Aug 10, 2018

Conversation

PeterPetrik
Copy link
Contributor

To be able to create some nice plots, we need a way how to get a (scalar) value from a dataset for particular point. The face (triangle) of the that contains point is identified by spatial index. Value is interpolated similarly to raster (contours) rendering of the mesh layer.

Probably the plots for mesh layers will not make it to QGIS 3.4, but see the example of the usage of the new function from python (Crayfish library for QGIS 3.4, not released yet = WIP)

mesh_plots

@@ -77,6 +80,12 @@ class CORE_EXPORT QgsTriangularMesh
//! Returns mapping between triangles and original faces
const QVector<int> &trianglesToNativeFaces() const ;

/**
* Returns triangle index that contains the given point, -1 if no such triangle exists
* It uses spatial indexing
Copy link
Member

Choose a reason for hiding this comment

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

\since

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -143,6 +146,15 @@ void QgsTriangularMesh::update( QgsMesh *nativeMesh, QgsRenderContext *context )
ENP_centroid( poly, cx, cy );
mNativeMeshFaceCentroids[i] = QgsMeshVertex( cx, cy );
}

// CALCULATE SPATIAL INDEX
for ( int i = 0; i < mTriangularMesh.faces.size(); ++i )
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what kind of datatype mTrisangularMesh.faces is, but if it's a container, better use an iterator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also need i few lines lower to add geometry to spatial index

Copy link
Member

Choose a reason for hiding this comment

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

If it's performance critical code, better iterate and use a separate counter. If not, no worries.

@@ -59,6 +59,12 @@ class QgsMeshLayerInterpolator : public QgsRasterInterface
int bandCount() const override;
QgsRasterBlock *block( int, const QgsRectangle &extent, int width, int height, QgsRasterBlockFeedback *feedback = nullptr ) override;


static double interpolateFromVerticesData( const QgsPointXY &p1,
Copy link
Member

Choose a reason for hiding this comment

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

I think this deserves some API docs too

*
* \since QGIS 3.4
*/
void activeScalarDatasetChanged( QgsMeshDatasetIndex index );
Copy link
Member

Choose a reason for hiding this comment

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

Is that a candidate for a const reference or a super trivial structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is 2 integers

Copy link
Member

Choose a reason for hiding this comment

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

In other places it's passed as const ref (e.g. QgsMeshDatasetValue QgsMeshLayer::datasetValue( const QgsMeshDatasetIndex &index, const QgsPointXY &point ) const), @nyalldawson I think you once investigated when it's appropriate to copy vs. ref?

bool isOnFaces = dataProvider()->datasetGroupMetadata( index ).isOnFaces();
if ( isOnFaces )
{
int native_face_index = mTriangularMesh->trianglesToNativeFaces().at( face_index );
Copy link
Collaborator

Choose a reason for hiding this comment

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

native_face_index -> camel case

* Returns NaN for NaN values, for values outside range or when
* triangular mesh is not yet initialized on given point
*/
QgsMeshDatasetValue datasetValue( const QgsMeshDatasetIndex &index, const QgsPointXY &point ) const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

\since QGIS 3.4

{
const QgsMeshFace &face = mTriangularMesh.faces.at( i ) ;
QgsGeometry geom = QgsMeshUtils::toGeometry( face, mTriangularMesh.vertices );
bool success = mSpatialIndex.insertFeature( i, geom.boundingBox() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of this just go

(void)mSpatialIndex.insertFeature( i, geom.boundingBox() );

const QList<QgsFeatureId> face_indexes = mSpatialIndex.intersects( QgsRectangle( point, point ) );
for ( QgsFeatureId fid : face_indexes )
{
int face_index = static_cast<int>( fid );
Copy link
Collaborator

Choose a reason for hiding this comment

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

camelCase


QgsGeometry QgsMeshUtils::toGeometry( const QgsMeshFace &face, const QVector<QgsMeshVertex> &vertices )
{
QVector<QgsPointXY> ring;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be more efficient to avoid the QgsPointXY methods here. Instead I'd suggest

QVector<QgsPoint> ring;
...

std::unique_ptr< QgsPolygon > polygon = qgis::make_unique< QgsPolygon >();
polygon.setExteriorRing( new QgsLineString( ring ) );
return QgsGeometry( std::move( polygon ) );

QVector<QgsPointXY> ring;
for ( int j = 0; j < face.size(); ++j )
{
int vertex_id = face[j];
Copy link
Collaborator

Choose a reason for hiding this comment

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

camel case

@nyalldawson nyalldawson added this to the 3.4 milestone Aug 10, 2018
@nirvn
Copy link
Contributor

nirvn commented Aug 10, 2018

@PeterPetrik , are you planning to add mesh layer support to the identify map tool? Users will probably expect it to work.

@PeterPetrik
Copy link
Contributor Author

@nirvn yes, but we would like to do it as a separate PR

*
* \since QGIS 3.4
*/
QgsMeshDatasetValue datasetValue( const QgsMeshDatasetIndex &index, const QgsPointXY &point ) const;
Copy link
Member

Choose a reason for hiding this comment

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

The "point" argument is in map coordinates rather than layer coordinates, right? That's worth noting in documentation (the assumption is that we pass layer coordinates)

Also, worth mentioning that it uses cached previously used triangular mesh and so if the layer has not been rendered previously (e.g. when used in a script) it would return invalid value

@@ -165,3 +178,31 @@ const QVector<int> &QgsTriangularMesh::trianglesToNativeFaces() const
return mTrianglesToNativeFaces;
}

int QgsTriangularMesh::faceIndexForPoint( const QgsPointXY &point ) const
{
const QList<QgsFeatureId> face_indexes = mSpatialIndex.intersects( QgsRectangle( point, point ) );
Copy link
Member

Choose a reason for hiding this comment

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

camelCase


if ( mTriangularMesh && dataProvider() && dataProvider()->isValid() && index.isValid() )
{
int face_index = mTriangularMesh->faceIndexForPoint( point ) ;
Copy link
Member

Choose a reason for hiding this comment

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

camelCase

/**
* \brief Returns whether dataset group data is defined on faces
*/
bool isOnFaces() const;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be nicer to replace isOnVertices() / isOnFaces() with a "type" enum. It would be easier to understand that "on vertices" and "on faces" are mutually exclusive

@wonder-sk
Copy link
Member

Tests fail only due to recently broken dbmanager test on master in f4d08eb

@wonder-sk wonder-sk merged commit ad4ddb1 into qgis:master Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants