Add new free-form digitizing using NURBSCurve #64358
Conversation
lbartoletti
force-pushed
the
feature/add_nurbscurve
branch
from
c28f033 to
62c29ef
Compare
🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. 🍎 MacOS Qt6 buildsDownload MacOS Qt6 builds of this PR for testing. |
|
Impressive work. I believe addition of a major new geometry type deserves a QEP to discuss various design decisions. Can we please have a QEP? The QEP should also provide details about NURBS geometry type - you have referenced the ISO standard, but I am not sure everyone is willing to pay 200+ EUR to download it. I would also like to suggest to break such a large PR to multiple smaller PRs that can be reviewed one by one. Reviewing 7K+ new lines of code (including a lot math) in one go is certainly going to be a challenge. Thank you 🙂 |
There was a problem hiding this comment.
Impressive work !
I've given the branch a try, and creating 2 NURBSCurve in a scratch layer of type NURBSCurve. When trying to save that layer as GeoPackage, I get:
Export to vector file /tmp/out.gpkg failed.
Error: Feature write errors:
Feature geometry not imported (OGR error: Pointer 'hGeom' is NULL in 'OGR_G_ImportFromWkb'.
)
Feature geometry not imported (OGR error: Pointer 'hGeom' is NULL in 'OGR_G_ImportFromWkb'.
)
Only 0 of 2 features written.
and this message from GDAL in stderr:
Warning 1: Layer new_scratch_layer relies on the 'gpkg_geom_' (http://www.geopackage.org/spec120/#extension_geometry_types) extension that should be implemented in order to read/write it safely, but is not currently. Some data may be missing while reading that layer, and updates are strongly discouraged.
| if ( QgsWkbTypes::isCurvedType( type ) && QgsWkbTypes::flatType( type ) != Qgis::WkbType::NurbsCurve ) | ||
| { | ||
| // Check if geometry contains NurbsCurve that needs conversion | ||
| bool hasNurbs = false; |
There was a problem hiding this comment.
couldn't you use QgsNurbsUtils::containsNurbsCurve() here ?
src/core/geometry/qgsnurbscurve.h
Outdated
|
|
||
| /** | ||
| * Sets the knot vector of the NURBS curve. | ||
| * \param knots knot vector (must have size = control points count + degree + 1) |
There was a problem hiding this comment.
it should be mentioned that the values in knots must be non-decreasing
By the way, do we need all those setters ? It doesn't look this PR use them at all. I would tend to think that a geometry instance must be (mostly) immutable.
| return false; | ||
| } | ||
|
|
||
| if ( mKnots.size() != n + mDegree + 1 ) |
There was a problem hiding this comment.
shouldn't we check that the value in mKnots are non-decreasing ? (otherwise findKnotSpan() could misbehave)
src/core/geometry/qgsnurbscurve.cpp
Outdated
| return false; | ||
| } | ||
|
|
||
| if ( o->mWeights.size() != mWeights.size() || o->mKnots.size() != mKnots.size() ) |
There was a problem hiding this comment.
can't you just compare mKnots != o->mKnots and mControlPoints != o-mControlPoints ? Not sure about QVector, but that would definitely work with a std::vector
| //! structure to keep information about a rubber band used for dragging of a NURBS curve | ||
| struct NurbsBand | ||
| { | ||
| QgsRubberBand *curveBand = nullptr; //!< Rubber band for the evaluated NURBS curve |
There was a problem hiding this comment.
use std::unique_ptr ? Applies to controlBand, mNurbsControlPolygonBand, mBezierTangentBands, mBezierAnchorMarkers, mBezierHandleMarkers
| maptools/qgsbezierdata.cpp | ||
| maptools/qgsbeziermarker.cpp | ||
|
|
||
| maptools/qgsmaptooledit.cpp |
There was a problem hiding this comment.
| maptools/qgsmaptooledit.cpp |
already present 2 lines below
| mHasR = true; | ||
| } | ||
|
|
||
| if ( qgsgeometry_cast<const QgsNurbsCurve *>( geom ) ) |
There was a problem hiding this comment.
all below logic could be simplified by using QgsNurbsUtils::containsNurbsCurve(), and setting mHasR = true as well
| if ( mode() == CapturePolygon ) | ||
| { | ||
| // For polygon, we need to close the curve - add first anchor as last anchor | ||
| // The NurbsCurve from asNurbsCurve is already properly formed |
There was a problem hiding this comment.
looks like there is missing code here
| return; | ||
| } | ||
|
|
||
| //polygons: bail out if there are not at least two vertices |
There was a problem hiding this comment.
assuming the code is correct
| //polygons: bail out if there are not at least two vertices | |
| //polygons: bail out if there are not at least three vertices |
|
Please don't merge before I've had a chance to review (which won't be till after new years break ends) |
lbartoletti
force-pushed
the
feature/add_nurbscurve
branch
from
62c29ef to
7b72dc3
Compare
Thanks 😊
I don't remember doing it in the past, and I don't think I've seen it happen. For what it's worth. The PR only adds a widely known geometry, NURBS. The other part is just inserting it into the existing code with some adjustments. But, once again, if it's really necessary, I'll open the QEP.
ISO standard is only for reference about WKB and some requirements.
On this point, I completely agree.
you're welcome 😊 |
Thanks 😊
That's weird, I don't have that in my demos/tests. I can't reproduce it, but I'll take a look after the christmas/newyear break. Many thanks for the review. 🙏 |
lbartoletti
added
Map Tools
|
@lbartoletti A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged. Please update the description (not the comments) with helpful description and screenshot to help the work from documentors. Thank you! |
Tests failed for Qt 6 (ALL_BUT_PROVIDERS - ubuntu)One or more tests failed using the build from commit 7b72dc3 layout_exportlayout_exportTest failed at test_layout_export at tests/src/python/test_selective_masking.py:1161 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export/layout_export.png (found 8 pixels different) layout_export_2_sources_maskinglayout_export_2_sources_maskingTest failed at test_layout_export_2_sources_masking at tests/src/python/test_selective_masking.py:1533 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_2_sources_masking/layout_export_2_sources_masking.png (found 6 pixels different) layout_export_w_blend_modelayout_export_w_blend_modeTest failed at test_layout_export_w_label_blend_mode at tests/src/python/test_selective_masking.py:1236 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_w_blend_mode/layout_export_w_blend_mode.png (found 287 pixels different) layout_export_w_rasterlayout_export_w_rasterTest failed at test_layout_export_w_raster at tests/src/python/test_selective_masking.py:1327 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_w_raster/layout_export_w_raster.png (found 6 pixels different) layout_export_markerline_maskedlayout_export_markerline_maskedTest failed at test_markerline_masked at tests/src/python/test_selective_masking.py:1817 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_markerline_masked/layout_export_markerline_masked.png (found 8 pixels different) The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
Tests failed for Qt 6 (ALL_BUT_PROVIDERS - fedora)One or more tests failed using the build from commit 7b72dc3 layout_exportlayout_exportTest failed at test_layout_export at tests/src/python/test_selective_masking.py:1161 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export/layout_export.png (found 8 pixels different) layout_export_2_sources_maskinglayout_export_2_sources_maskingTest failed at test_layout_export_2_sources_masking at tests/src/python/test_selective_masking.py:1533 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_2_sources_masking/layout_export_2_sources_masking.png (found 6 pixels different) layout_export_w_blend_modelayout_export_w_blend_modeTest failed at test_layout_export_w_label_blend_mode at tests/src/python/test_selective_masking.py:1236 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_w_blend_mode/layout_export_w_blend_mode.png (found 287 pixels different) layout_export_w_rasterlayout_export_w_rasterTest failed at test_layout_export_w_raster at tests/src/python/test_selective_masking.py:1327 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_w_raster/layout_export_w_raster.png (found 6 pixels different) layout_export_markerline_maskedlayout_export_markerline_maskedTest failed at test_markerline_masked at tests/src/python/test_selective_masking.py:1817 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_markerline_masked/layout_export_markerline_masked.png (found 8 pixels different) The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
| Constructor for an empty NURBS curve geometry. | ||
| %End | ||
|
|
||
| QgsNurbsCurve( const QVector<QgsPoint> &ctrlPoints, int degree, const QVector<double> &knots, const QVector<double> &weights ); |
There was a problem hiding this comment.
| QgsNurbsCurve( const QVector<QgsPoint> &ctrlPoints, int degree, const QVector<double> &knots, const QVector<double> &weights ); | |
| QgsNurbsCurve( const QVector<QgsPoint> &controlPoints, int degree, const QVector<double> &knots, const QVector<double> &weights ); |
| points) | ||
| %End | ||
|
|
||
| virtual QgsCurve *clone() const /Factory/; |
There was a problem hiding this comment.
| virtual QgsCurve *clone() const /Factory/; | |
| virtual QgsNurbsCurve *clone() const /Factory/; |
| virtual QgsCurve *clone() const /Factory/; | ||
|
|
||
|
|
||
| QgsPoint evaluate( double t ) const; |
There was a problem hiding this comment.
Would be nice to raise a Python ValueError here if t < 0 or > 1
| :return: point on the curve at parameter t | ||
| %End | ||
|
|
||
| bool isBezier() const; |
There was a problem hiding this comment.
Can you add HoldGIL annotation to these too?
| :param degree: curve degree (typically 1-3) | ||
| %End | ||
|
|
||
| const QVector<QgsPoint> &controlPoints() const /HoldGIL/; |
There was a problem hiding this comment.
| const QVector<QgsPoint> &controlPoints() const /HoldGIL/; | |
| QVector<QgsPoint> controlPoints() const /HoldGIL/; |
| * Returns the NURBS curve and sets \a localIndex to the control point index | ||
| * within that curve. Returns NULLPTR if the vertex is not part of a NURBS curve. | ||
| */ | ||
| static QgsNurbsCurve *findMutableNurbsCurveForVertex( |
There was a problem hiding this comment.
could we just name this findNurbsCurveForVertex and SIP_SKIP the const version? SIP ignores the const qualifier for pointers anyway.
| * within that curve. Returns NULLPTR if the vertex is not part of a NURBS curve. | ||
| */ |
There was a problem hiding this comment.
Needs \param and \returns for sipify to do its magic when SIP_OUT is used
| explicit QgsPointLocator_Stream( const QLinkedList<RTree::Data *> &dataList ) | ||
| : mDataList( dataList ) | ||
| , mIt( mDataList ) | ||
| { } |
There was a problem hiding this comment.
Can you revert the unrelated formatting changes?
| explicit QgsPointLocator( QgsVectorLayer *layer, const QgsCoordinateReferenceSystem &destinationCrs = QgsCoordinateReferenceSystem(), | ||
| const QgsCoordinateTransformContext &transformContext = QgsCoordinateTransformContext(), | ||
| const QgsRectangle *extent = nullptr ); | ||
| explicit QgsPointLocator( QgsVectorLayer *layer, const QgsCoordinateReferenceSystem &destinationCrs = QgsCoordinateReferenceSystem(), const QgsCoordinateTransformContext &transformContext = QgsCoordinateTransformContext(), const QgsRectangle *extent = nullptr ); |
There was a problem hiding this comment.
revert unrelated formatting changes
| { Qgis::WkbType::CircularStringM, WkbEntry( QLatin1String( "CircularStringM" ), false, Qgis::WkbType::MultiCurveM, Qgis::WkbType::CircularStringM, Qgis::WkbType::CircularString, Qgis::GeometryType::Line, false, true ) }, | ||
| { Qgis::WkbType::CircularStringZM, WkbEntry( QLatin1String( "CircularStringZM" ), false, Qgis::WkbType::MultiCurveZM, Qgis::WkbType::CircularStringZM, Qgis::WkbType::CircularString, Qgis::GeometryType::Line, true, true ) }, | ||
| //nurbscurve | ||
| { Qgis::WkbType::NurbsCurve, WkbEntry( QLatin1String( "NurbsCurve" ), false, Qgis::WkbType::MultiCurve, Qgis::WkbType::NurbsCurve, Qgis::WkbType::NurbsCurve, Qgis::GeometryType::Line, false, false ) }, |
There was a problem hiding this comment.
Please extend the tests in test_qgsgeometry.py testWkbTypes accordingly
|
@lbartoletti I've reviewed only the core changes above -- in general it's looking great, nice work! Just some minor changes requested for those. Let me know if you are planning on stripping the gui/app parts out to a separate PR or if they need reviewing here too |
5 participants
Description
This PR adds support for NURBS curves (Non-Uniform Rational B-Splines) in QGIS.
NURBS are a mathematical model used to represent smooth curves and surfaces using control points, weights, and a knot vector. They generalize Bézier curves and B-splines, providing fine-grained control over shape through control point weights.
The implementation follows the ISO/IEC 13249-3:2016 standard. This geometry type is already supported by SFCGAL, and its integration into PostGIS is currently under development.
Several QGIS plugins already provide Bézier curve tools. This implementation goes further by offering full NURBS support, including Bézier curves, B-splines, and rational NURBS curves.
Development was guided by user feedback and customer requests.
User Documentation
Digitizing Menu
A new NURBS digitizing mode has been added to the digitizing techniques menu, with two sub-modes:
Control Points Mode
This mode allows direct placement of NURBS control points.
nurbs_control_points.mp4
Poly-Bézier Mode
This mode provides an intuitive digitizing experience similar to vector drawing software:
A contextual help banner is displayed to guide the user.
nurbs_bezier_digitize.mp4
Format Compatibility
NurbsCurvegeometry (hope for 3.7.0)Here's an example of how it's segmented in linestring/polygon.
nurbs_linestring.mp4
nurbs_polygon.mp4
Editing Tools
Standard editing tools fully support NURBS geometries:
nurbs_add_fill.mp4
nurbs_tools.mp4
Vertex Tool Support
The vertex tool supports NURBS geometries:
nurbs_vertextool.mp4
3D
NURBS supports Z (and M). So you can view your drawing in 3D. Example with a simple spiral here. But it works for some landscape/urban projects too.
nurbs_spiral_3D.mp4
Development
Some notes on the main parts for developper.
Core
QgsNurbsCurveclass inheriting fromQgsCurveQgis::WkbType::NurbsCurveQgsNurbsUtilsfor NURBS detection and extractionGUI
Significant changes in
QgsMapToolCapture:Qgis::CaptureTechnique::NurbsCurveQgsBezierDataandQgsBezierMarkerclasses for poly-Bézier handlingQgsMapToolCaptureRubberBandto render NURBS curvesApp
QgsMapToolsDigitizingTechniqueManager: Added NURBS mode with options (mode, degree)QgsVertexTool: Support for editing NURBS control points and weightsQgsVertexEditor: Weight column for NURBS geometriesProviders
NurbsCurvetable (can be removed until we haven't merged nurbs)Tests
Sponsored by Stadt Frankfurt am Main