-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Feature/add nurbscurve core part #64521
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/add nurbscurve core part #64521
Conversation
Thank you! I'll review ASAP |
|
Ok, I've re-reviewed and copied over the outstanding comments from the original PR which still need action. |
🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. 🍎 MacOS Qt6 buildsDownload MacOS Qt6 builds of this PR for testing. |
0113519 to
95e2aeb
Compare
Tests failed for Qt 6 (ALL_BUT_PROVIDERS - fedora)One or more tests failed using the build from commit 95e2aeb 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 - ubuntu)One or more tests failed using the build from commit 95e2aeb 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. |
269b5fa to
6fea194
Compare
|
LGTM -- except for one last comment, and the selective masking control image changes. Can you replace with the images with test mask updates instead? |
6fea194 to
48c735d
Compare
|
It seems to me that the changes in should go in a follow up PR related to digitizing, also subject to discussion on the promised upcoming QEP. It would also be helpful for fellow reviewers if you refrained from force pushing during the review process. |
I did it, I think, it's ok with my last commits. |
|
Many thanks to both of you @nyalldawson @rouault, for your careful review. I’m sorry I missed some of your remarks and for the mess I created while juggling merges and conflicts. |
Was this commit pushed? I still see the replacement control images, not masks. Otherwise it's a +1 to merge from me. Lovely work here, adding a new geometry class like this is very tricky 💯 |
|
@lbartoletti looks like the control image changes are still here -- can you revert those and leave JUST the mask updates? |
co-authored: Nyall Dawson <[email protected]>
…interpolatePointOnCubicBezier The previous algorithm accumulated error with += and -=, new algorithm is more robust and numercially stable
…GeometryUtilsBase and use it
Add missing mask files
9cbaa81 to
6d70287
Compare
|
Thanks @lbartoletti -- nothing else from me! |
Ah, indeed! it's fixup. Thanks!
Many thanks 😊 And thanks again for your review with even! |
Can I please have a comment regarding this? |
Sure! Unfortunately, some parts are "intertwined". But after looking at the commits, I can remove these two, |
I'm sorry I was not clear regarding |
|
ok, done |
Description
This PR is the extraction of the CORE part of the free-forms/NURBS functionality. I have made clean commits again, incorporating comments from @nyalldawson and @rouault . I hope I haven't made any mistakes in my multiple rebases/conflicts...
The other parts will be opened after.
Thank you again for your reviews and positive comments on the previous PR 🙏 💗 .
I am leaving #64358 open for overview and documentation purposes.
Sponsored by Stadt Frankfurt am Main