Skip to content

Use curve to digitize Polylines/Polygons #5842

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

qsavoye
Copy link
Contributor

@qsavoye qsavoye commented Nov 21, 2024

Use curve to digitize Polylines/Polygons

test

  1. Active Curve mode during Digitization of a Polygon
  2. Add a first vertex to define the center point of the arc
  3. Add a second vertex to stop the arc

Precision of the final arc can be set in QField Parameters page (slider)

height: visible ? 40 : 0
padding: 2
round: true
visible: dashBoard.activeLayer && (dashBoard.activeLayer.geometryType() === Qgis.GeometryType.Polygon || dashBoard.activeLayer.geometryType() === Qgis.GeometryType.Line)
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 we want this to be dependent on QgsWkbTypes.isCurvedType()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my example , my layer is not a curve type at all and i use QgsCompoundCurve::curveToLine function to set geometry drawn into my none curved layer. This helps me to drawn faster instead of set vertex by vertex.

But you are right i am not taking into account the possibility to set directly the qgscompoundcurve geometry into the new feature.

How do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I was mostly seeing this in the context of surveying / cadastre, where curves are often a requirement. My main motivation here would be to keep the interface as light as possible and ask users to have a curve layer if they want this feature. What do you think?

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 am using it to draw underground maps, such as caves or old quarries. My polygon layer corresponds to some galleries and i no need to have a specific curved layer but some times digitizing arcs like in qgis help me.

Maybe we can allow it only for curved layer (QgsWkbTypes.isCurvedType()) or for advanced digitizing user ( which could be set in settings).

Tell me what you think ?

Sorry for the delay, i had some stuff to do last week.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting job you have!

I'd be interested to hear @nirvn's point of view

Copy link
Member

Choose a reason for hiding this comment

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

@m-kuhn , @qsavoye , hey there, sorry for the delay in chipping in.

I do think the feature is useful; I would indeed prefer if this option was only visible for curved type layers, I feel that'd be a good way to enable this while not cluttering UI by limiting to an explicit intent by the user / project manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m-kuhn @nirvn,
Ok with your reviews.
I will patch it for only allowing layer with QgsWkbTypes.isCurvedType() as soon as I have time

@@ -242,7 +242,19 @@ QfVisibilityFadingRow {
if (Number(rubberbandModel.geometryType) === Qgis.GeometryType.Point || Number(rubberbandModel.geometryType) === Qgis.GeometryType.Null) {
confirm();
} else {
addVertex();
if (settings.valueBool("/QField/Digitizing/CurveEdition", false) == true) {
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 we need an additional check here to avoid curved edition when people are freehand drawing, otherwise worlds will collide.

@m-kuhn
Copy link
Member

m-kuhn commented Dec 25, 2024

@9ls1 I noted you added a 👍 to this, do you have a specific use case in mind for which this would be interesting?

@9ls1
Copy link

9ls1 commented Dec 25, 2024

@m-kuhn No, just in general thinking about saving time digitizing shapes like the one shown here and curved roads.

@qsavoye qsavoye requested review from nirvn and m-kuhn March 25, 2025 16:30
@nirvn
Copy link
Member

nirvn commented Mar 30, 2025

@qsavoye , you have not been forgotten :) I'll review this in the coming day. Thanks for your patience.

@@ -197,14 +215,19 @@ class QFIELD_CORE_EXPORT RubberbandModel : public QObject
void measureValueChanged();

private:
QVector<QgsPoint> mPointList;
QgsCompoundCurve mCompoundCurve;
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little bit worried of the extra cost of moving from a simple QVector to a QgsAbstractGeometry-based curve here. I am wondering whether we should keep both the fast point list vector (used by our tracking system among other things), and have the mCompoundCurve as an alternative path for curved geometries.

@@ -60,7 +72,7 @@ QgsPointSequence RubberbandModel::pointSequence( const QgsCoordinateReferenceSys
QgsPointSequence sequence;
QgsCoordinateTransform ct( mCrs, crs, QgsProject::instance()->transformContext() );

for ( const QgsPoint &pt : mPointList )
for ( const QgsPoint &pt : vertices() )
Copy link
Member

Choose a reason for hiding this comment

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

Let's do that instead.

Suggested change
for ( const QgsPoint &pt : vertices() )
const QVector<QgsPoint> vertices = vertices();
for ( const QgsPoint &pt : vertices )

@@ -97,7 +109,7 @@ QVector<QgsPointXY> RubberbandModel::flatPointSequence( const QgsCoordinateRefer

QgsCoordinateTransform ct( mCrs, crs, QgsProject::instance()->transformContext() );

for ( const QgsPoint &pt : mPointList )
for ( const QgsPoint &pt : vertices() )
Copy link
Member

Choose a reason for hiding this comment

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

See above.

@@ -779,6 +779,10 @@ void FeatureModel::applyGeometry()
geometry = deduplicatedGeometry;
}

if ( QgsWkbTypes::isCurvedType( layer()->wkbType() ) == true )
Copy link
Member

Choose a reason for hiding this comment

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

You'll need more than this to preserve the curves here. You'll need to edit the Geometry::asQgsGeometry (

QgsGeometry Geometry::asQgsGeometry() const
) function to preserve curves passed on by the rubberband when appropriate.

@@ -383,3 +383,11 @@ FeatureIterator LayerUtils::createFeatureIteratorFromExpression( QgsVectorLayer
const QgsFeatureRequest request = QgsFeatureRequest( QgsExpression( expression ) );
return FeatureIterator( layer, request );
}

bool LayerUtils::isCurvedGeometry( QgsVectorLayer *layer )
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: we should make the QgswkbTypes' isCurvedType Q_INVOKABLE upstream, that way we don't need an extra utils function.

That being said, I think I'd rather have that in the geometryutils, and on the QML side of things you can call a vector layer's wkbType() function (it's invokable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nirvn
Do you prefer : make a patch when compiling qgis to modify QgsWkbTypes.h and erase what I did on LayerUtils or move the method into GeometryUtils ?

Copy link
Member

Choose a reason for hiding this comment

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

@qsavoye , I'd love for you to submit a patch in QGIS upstream (we're 5 days away from 3.44 release), then we can update QField to 3.44 when released and we'll be able to make sure of that newly-made invokable function.

Copy link
Member

Choose a reason for hiding this comment

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

@qsavoye , I've taken care of upstream here: qgis/QGIS#62306

@@ -188,6 +188,11 @@ void VertexModel::refreshGeometry()
}
}

if ( QgsWkbTypes::isCurvedType( geom.wkbType() ) == true )
{
geom.convertToStraightSegment();
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried about this change, did you give it a try? Was that giving you usable scenarios?

@nirvn
Copy link
Member

nirvn commented Apr 17, 2025

@qsavoye , are you planning to work on this further?

@qsavoye
Copy link
Contributor Author

qsavoye commented Apr 17, 2025

@qsavoye , are you planning to work on this further?

@nirvn , yes but i will be unavailable to work on it for at least a month :(

@nirvn
Copy link
Member

nirvn commented Apr 17, 2025

@qsavoye , no problem, take your time. You're contributions are appreciated, just glad to hear you're planning to come back to it :)

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

Successfully merging this pull request may close these issues.

4 participants