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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[geometry snapper] Fix wrong point-to-segment distance within maths #56426

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

nirvn
Copy link
Contributor

@nirvn nirvn commented Feb 20, 2024

Description

This PR fixes a pretty serious issue with our QgsGeometrySnapper class whereas snapping of point geometry to line segments would often fail due to a bogus distance to line calculation.

We were checking whether a given tolerance/distance was within a squared distance value, leading to failures 馃槺

Fixes #56424 , #50121 , and likely other issues. A test has been added.

@github-actions github-actions bot added this to the 3.36.0 milestone Feb 20, 2024
@nirvn nirvn added Processing Relating to QGIS Processing framework or individual Processing algorithms backport release-3_34 labels Feb 20, 2024
Copy link

github-actions bot commented Feb 20, 2024

馃獰 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit bc957d6)

@nirvn nirvn force-pushed the geometrysnapper_distance_fix branch from 74aab4e to b1d22b9 Compare February 21, 2024 02:17
@nirvn
Copy link
Contributor Author

nirvn commented Feb 21, 2024

@nyalldawson , addressed.

Copy link

Tests failed for Qt 6

One or more tests failed using the build from commit bc957d6

composermap_render (render)

composermap_render

Test failed at render at tests/src/core/testqgslayoutmap.cpp:185

Rendered image did not match tests/testdata/control_images/composer_map/expected_composermap_render/expected_composermap_render.png (found 13824 pixels different)

composermap_overview_center (overviewMapCenter)

composermap_overview_center

Test failed at overviewMapCenter at tests/src/core/testqgslayoutmapoverview.cpp:224

Rendered image did not match tests/testdata/control_images/composer_mapoverview/expected_composermap_overview_center/expected_composermap_overview_center.png (found 116 pixels different)

flat_terrain_2 (testFlatTerrain)

flat_terrain_2

Test failed at testFlatTerrain at tests/src/3d/testqgs3drendering.cpp:316

Rendered image did not match tests/testdata/control_images/3d/expected_flat_terrain_2/expected_flat_terrain_2.png (found 2725 pixels different)

dem_terrain_1 (testDemTerrain)

dem_terrain_1

Test failed at testDemTerrain at tests/src/3d/testqgs3drendering.cpp:363

Rendered image did not match tests/testdata/control_images/3d/expected_dem_terrain_1/expected_dem_terrain_1.png (found 1603 pixels different)

4978_line_rendering_2 (testEpsg4978LineRendering)

4978_line_rendering_2

Test failed at testEpsg4978LineRendering at tests/src/3d/testqgs3drendering.cpp:1293

Rendered image did not match tests/testdata/control_images/3d/expected_4978_line_rendering_2/expected_4978_line_rendering_2.png (found 128045 pixels different)

phong_shading (testPhongShading)

phong_shading

Test failed at testPhongShading at tests/src/3d/testqgs3drendering.cpp:503

Rendered image did not match tests/testdata/control_images/3d/expected_phong_shading/expected_phong_shading.png (found 55 pixels different)

metal_rough (testExtrudedPolygonsMetalRoughShading)

metal_rough

Test failed at testExtrudedPolygonsMetalRoughShading at tests/src/3d/testqgs3drendering.cpp:649

Rendered image did not match tests/testdata/control_images/3d/expected_metal_rough/expected_metal_rough.png (found 54 pixels different)

line_rendering_2 (testLineRendering)

line_rendering_2

Test failed at testLineRendering at tests/src/3d/testqgs3drendering.cpp:761

dem_terrain_filtered_1 (testFilteredDemTerrain)

dem_terrain_filtered_1

Test failed at testFilteredDemTerrain at tests/src/3d/testqgs3drendering.cpp:1370

Rendered image did not match tests/testdata/control_images/3d/expected_dem_terrain_filtered_1/expected_dem_terrain_filtered_1.png (found 3886 pixels different)

composermap_overview_center

composermap_overview_center

Test failed at testOverviewMapCenter at tests/src/python/test_qgslayoutmapoverview.py:161

Rendered image did not match tests/testdata/control_images/composer_mapoverview/expected_composermap_overview_center/expected_composermap_overview_center.png (found 116 pixels different)

composermap_overview_belowmap

composermap_overview_belowmap

Test failed at testOverviewStacking at tests/src/python/test_qgslayoutmapoverview.py:333

Rendered image did not match tests/testdata/control_images/composer_mapoverview/expected_composermap_overview_belowmap/expected_composermap_overview_belowmap.png (found 580 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.

@nyalldawson
Copy link
Collaborator

Unrelated test failures

@nyalldawson nyalldawson merged commit f6e073f into qgis:master Feb 21, 2024
26 of 28 checks passed
@qgis-bot
Copy link
Collaborator

The backport to release-3_34 failed:

The process '/usr/bin/git' failed with exit code 1
stderr
error: could not apply bc957d68b00... Address review
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

stdout
Auto-merging src/analysis/vector/qgsgeometrysnapper.cpp
Auto-merging src/analysis/vector/qgsgeometrysnapper.h
[backport-56426-to-release-3_34 f60f3103028] [geometry snapper] Fix wrong point-to-segment distance within maths
 Author: Mathieu Pellerin <nirvn.asia@gmail.com>
 Date: Tue Feb 20 20:27:45 2024 +0700
 3 files changed, 10 insertions(+), 5 deletions(-)
Auto-merging src/analysis/vector/qgsgeometrysnapper.cpp
CONFLICT (content): Merge conflict in src/analysis/vector/qgsgeometrysnapper.cpp
Auto-merging src/analysis/vector/qgsgeometrysnapper.h

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-3_34 release-3_34
# Navigate to the new working tree
cd .worktrees/backport-release-3_34
# Create a new branch
git switch --create backport-56426-to-release-3_34
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick b1d22b9e0a6e8a2fe6c393817fb1319de44d9a21,bc957d68b004c078a163e315d292fdc548edccdd
# Push it to GitHub
git push --set-upstream origin backport-56426-to-release-3_34
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-3_34

Then, create a pull request where the base branch is release-3_34 and the compare/head branch is backport-56426-to-release-3_34.

@qgis-bot qgis-bot added the failed backport The automated backport attempt failed, needs a manual backport label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-3_34 failed backport The automated backport attempt failed, needs a manual backport Processing Relating to QGIS Processing framework or individual Processing algorithms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QGIS: Snap geometries to layer - not all points are snapping
6 participants