Skip to content

fix(CorridorScanComplexItem): Fix the signal emission logic for - #14725

Closed
cuav-zhang3 wants to merge 1 commit into
mavlink:masterfrom
cuav-zhang3:fix-corridorScan-Line-Segment-Connection
Closed

fix(CorridorScanComplexItem): Fix the signal emission logic for#14725
cuav-zhang3 wants to merge 1 commit into
mavlink:masterfrom
cuav-zhang3:fix-corridorScan-Line-Segment-Connection

Conversation

@cuav-zhang3

@cuav-zhang3 cuav-zhang3 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

coordinate specification state changes

Add a cache variable to record the previous coordinate specification status, detect status changes during corridor polygon reconstruction, and emit corresponding signals to ensure the task controller correctly updates the waypoint connection lines.

Description

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)
  • CI/Build changes
  • Other

Testing

  • Tested locally
  • Added/updated unit tests
  • Tested with simulator (SITL)
  • Tested with hardware

Platforms Tested

  • Linux
  • Windows
  • macOS
  • Android
  • iOS

Flight Stacks Tested

  • PX4
  • ArduPilot

Screenshots

test1 test2

Checklist

  • I have read the Contribution Guidelines
  • I have read the Code of Conduct
  • My code follows the project's coding standards
  • I have added tests that prove my fix/feature works
  • New and existing unit tests pass locally

Related Issues


By submitting this pull request, I confirm that my contribution is made under the terms of the project's dual license (Apache 2.0 and GPL v3).

coordinate specification state changes

Add a cache variable to record the previous coordinate specification
status, detect status changes during corridor polygon reconstruction,
and emit corresponding signals to ensure the task controller correctly
updates the waypoint connection lines.
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 32.63%. Comparing base (f29efd3) to head (7d68c86).
⚠️ Report is 213 commits behind head on master.

Files with missing lines Patch % Lines
src/MissionManager/CorridorScanComplexItem.cc 40.00% 0 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14725      +/-   ##
==========================================
+ Coverage   25.47%   32.63%   +7.16%     
==========================================
  Files         769      783      +14     
  Lines       65912    67518    +1606     
  Branches    30495    31267     +772     
==========================================
+ Hits        16788    22032    +5244     
+ Misses      37285    30632    -6653     
- Partials    11839    14854    +3015     
Flag Coverage Δ
unittests 32.63% <40.00%> (+7.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/MissionManager/CorridorScanComplexItem.h 27.27% <ø> (ø)
src/MissionManager/CorridorScanComplexItem.cc 32.81% <40.00%> (+11.00%) ⬆️

... and 475 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94ab3c6...7d68c86. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

Build Results

Platform Status

Platform Status Details
Linux Passed View
Windows Passed View
MacOS Passed View
Android Passed View

All builds passed.

Pre-commit

Check Status Details
pre-commit Failed (non-blocking) View

Pre-commit hooks: 0 passed, 0 failed, 0 skipped.

Test Results

linux-coverage-integration: 37 passed, 0 skipped
linux-coverage-unit: 114 passed, 0 skipped
linux-sanitizers-integration: 20 passed, 0 skipped
linux-sanitizers-unit: 114 passed, 0 skipped
Total: 285 passed, 0 skipped

Code Coverage

Coverage Baseline Change
69.3% 69.3% +0.1%

Artifact Sizes

Artifact Size Δ from master
QGroundControl 221.78 MB -4.44 MB (decrease)
QGroundControl-aarch64 179.35 MB +0.00 MB (increase)
QGroundControl-installer-AMD64 137.84 MB -0.45 MB (decrease)
QGroundControl-installer-AMD64-ARM64 80.36 MB -0.22 MB (decrease)
QGroundControl-installer-ARM64 109.12 MB +0.01 MB (increase)
QGroundControl-linux 87.02 MB -78.72 MB (decrease)
QGroundControl-mac 87.02 MB No change
QGroundControl-x86_64 191.57 MB -0.01 MB (decrease)
Total size decreased by 83.83 MB

Updated: 2026-07-28 03:04:25 UTC • Commit: 7d68c86 • Triggered by: Android

@DonLakeFlyer DonLakeFlyer added this to the Release V5.1 milestone Jul 29, 2026
@DonLakeFlyer

Copy link
Copy Markdown
Contributor

The repro for this is:

  • Set home
  • Add takeoff
  • Add corridor scan
  • Trace - add two points
  • Done tracing

Now the corridor scan doesn't connect to the takeoff item

@DonLakeFlyer

Copy link
Copy Markdown
Contributor

@cuav-zhang3 Thanks for the pull. I'm going to reorganize this a bit to make the fix clearer.

@DonLakeFlyer

Copy link
Copy Markdown
Contributor

Thanks for tracking this down — the diagnosis is correct: specifiesCoordinate() is dynamic but specifiesCoordinateChanged was never emitted, so MissionController never re-evaluated the waypoint connector lines.

I'm closing this in favor of #14749, which fixes the same bug with a slightly different structure:

  • Emission tied to the actual state change. specifiesCoordinate() depends only on the polyline vertex count, so the emit lives in a dedicated _updateSpecifiesCoordinate() slot connected to QGCMapPolyline::countChanged, rather than inside _rebuildCorridorPolygon(). That slot is also triggered by corridor width changes (which can't affect specifiesCoordinate()), and a future refactor of the polygon rebuild couldn't silently drop the signal emission.
  • countChanged covers every mutation pathappendVertex, clear(), and setPath/JSON load all route through it, so both the false→true and true→false transitions are always signaled.
  • No constructor cache seeding needed. The _specifiesCoordinate = specifiesCoordinate() line in this PR runs at a point where the polyline is guaranteed empty (the KML/SHP load happens after the connects), so it was a no-op; fix(CorridorScanComplexItem): emit specifiesCoordinateChanged when polyline validity changes #14749 instead anchors the cache once after the signal connections are made.
  • Adds a regression unit test (_testSpecifiesCoordinateChanged) that fails on master and passes with the fix.

Appreciate the report and the analysis — the screenshots made the repro easy to confirm.

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.

2 participants