feat(waypoint radius): Add waypoint radius setting & map visual - #14579
Open
AhmWael wants to merge 2 commits into
Open
feat(waypoint radius): Add waypoint radius setting & map visual#14579AhmWael wants to merge 2 commits into
AhmWael wants to merge 2 commits into
Conversation
Introduce a Waypoint Radius Fact and UI/visual support. MissionSettingsItem gained a waypointRadius Fact (default 50 m) with metadata and sync logic to vehicle parameters (WP_RADIUS / WPNAV_RADIUS / NAV_ACC_RAD). MissionController now saves/loads waypointRadius in mission JSON. PlanMasterController writes the parameter to the vehicle during send. SimpleMissionItem exposes waypointRadius and triggers visual updates. QML editors (MissionDefaultsEditor, MissionSettingsEditor) and SimpleItemMapVisual add controls and an interactive map circle to view/edit radius. Unit test added to verify propagation.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a global “Waypoint Radius” mission setting (Fact) with Plan View editors and map visualization, and persists it in .plan files while attempting to synchronize it with vehicle parameters.
Changes:
- Introduces a
waypointRadiusFact onMissionSettingsItem, serializes it to/from the root mission JSON, and surfaces it in Plan View editors. - Adds waypoint-radius circle visuals to each waypoint in
SimpleItemMapVisual.qml, including interactive drag-to-edit behavior. - Wires
SimpleMissionItemto expose the global waypoint radius and adds a unit test exercising the new API.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MissionManager/SimpleMissionItemTest.h | Adds a new test slot for waypoint radius. |
| test/MissionManager/SimpleMissionItemTest.cc | Adds a basic waypoint radius test and test setup changes. |
| src/PlanView/SimpleItemMapVisual.qml | Adds loader/component to render/edit waypoint radius circles on the map. |
| src/PlanView/MissionSettingsEditor.qml | Adds a waypoint radius editor field for mission settings. |
| src/PlanView/MissionDefaultsEditor.qml | Adds a waypoint radius editor field in mission defaults UI. |
| src/MissionManager/SimpleMissionItem.h | Exposes waypoint radius + visibility properties/signals to QML. |
| src/MissionManager/SimpleMissionItem.cc | Implements lookup/notification for the global waypoint radius Fact. |
| src/MissionManager/PlanMasterController.cc | Attempts to push waypoint radius into a vehicle parameter before mission upload. |
| src/MissionManager/MissionSettingsItem.h | Adds waypoint radius Fact API and vehicle-sync slot/state. |
| src/MissionManager/MissionSettingsItem.cc | Creates waypoint radius Fact metadata/default and implements vehicle parameter sync. |
| src/MissionManager/MissionController.cc | Saves/loads waypointRadius in the mission root JSON. |
Comment on lines
+29
to
31
| MissionSettingsItem* settingsItem = new MissionSettingsItem(planController(), false /* flyView */); | ||
| planController()->missionController()->visualItems()->append(settingsItem); | ||
| _simpleItem = new SimpleMissionItem(planController(), false /* flyView */, missionItem); |
Comment on lines
+274
to
+278
| static Vehicle* lastVehicle = nullptr; | ||
| if (lastVehicle != vehicle) { | ||
| if (lastVehicle && lastVehicle->parameterManager()) { | ||
| disconnect(lastVehicle->parameterManager(), &ParameterManager::parametersReadyChanged, this, &MissionSettingsItem::_syncWaypointRadiusWithVehicle); | ||
| disconnect(lastVehicle->parameterManager(), &ParameterManager::factAdded, this, &MissionSettingsItem::_syncWaypointRadiusWithVehicle); |
Comment on lines
+303
to
+307
| connect(&_waypointRadiusFact, &Fact::valueChanged, this, [paramFact](QVariant value){ | ||
| if (paramFact->rawValue() != value) { | ||
| paramFact->setRawValue(value); | ||
| } | ||
| }); |
Comment on lines
+297
to
+301
| if (_waypointRadiusFact.rawValue().toDouble() != 50.0) { | ||
| paramFact->setRawValue(_waypointRadiusFact.rawValue()); | ||
| } else if (paramFact->rawValue().toDouble() > 0.0) { | ||
| _waypointRadiusFact.setRawValue(paramFact->rawValue()); | ||
| } |
Comment on lines
+1162
to
+1166
| bool SimpleMissionItem::showWaypointRadius(void) const | ||
| { | ||
| Fact* fact = const_cast<SimpleMissionItem*>(this)->waypointRadius(); | ||
| return command() == MAV_CMD_NAV_WAYPOINT && fact && fact->rawValue().toDouble() > 0.0; | ||
| } |
Contributor
Author
|
@DonLakeFlyer Can you check this? |
Contributor
|
We're in bug fix only more for 5.1 now. So this will have to wait for 5.2. First step is dealing with Copilot review. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/MissionManager/MissionSettingsItem.cc:284
- Using a function-local static
lastVehiclemakes vehicle tracking shared across all MissionSettingsItem instances and can become dangling if the previous Vehicle is deleted. Also, this code reconnects to a new vehicle but never disconnects the existing Fact<->param Fact connections, so switching controllerVehicle can leave multiple vehicles being kept in sync unintentionally.
static Vehicle* lastVehicle = nullptr;
if (lastVehicle != vehicle) {
if (lastVehicle && lastVehicle->parameterManager()) {
disconnect(lastVehicle->parameterManager(), &ParameterManager::parametersReadyChanged, this, &MissionSettingsItem::_syncWaypointRadiusWithVehicle);
disconnect(lastVehicle->parameterManager(), &ParameterManager::factAdded, this, &MissionSettingsItem::_syncWaypointRadiusWithVehicle);
}
lastVehicle = vehicle;
_vehicleWaypointRadiusConnected = false;
connect(vehicle->parameterManager(), &ParameterManager::parametersReadyChanged, this, &MissionSettingsItem::_syncWaypointRadiusWithVehicle);
connect(vehicle->parameterManager(), &ParameterManager::factAdded, this, &MissionSettingsItem::_syncWaypointRadiusWithVehicle);
}
test/MissionManager/SimpleMissionItemTest.cc:30
- SimpleMissionItem::waypointRadius() assumes MissionSettingsItem is at visualItems()[0]. The test currently appends the settings item, which would break if the list is non-empty (and diverges from production code which inserts at index 0).
MissionSettingsItem* settingsItem = new MissionSettingsItem(planController(), false /* flyView */);
planController()->missionController()->visualItems()->append(settingsItem);
src/MissionManager/MissionSettingsItem.cc:307
- The lambda connected to
_waypointRadiusFactcapturesparamFactwhile the receiver/context isthis. If the vehicle disconnects andparamFactis destroyed, later offline edits to_waypointRadiusFactcan dereference a stale pointer.
connect(&_waypointRadiusFact, &Fact::valueChanged, this, [paramFact](QVariant value){
if (paramFact->rawValue() != value) {
paramFact->setRawValue(value);
}
});
src/MissionManager/SimpleMissionItem.cc:1187
- showWaypointRadius depends on the mission command, but showWaypointRadiusChanged is only emitted when the waypointRadius Fact changes. If the command changes to/from MAV_CMD_NAV_WAYPOINT, QML bindings using showWaypointRadius may not update.
if (fact && !_waypointRadiusConnected) {
_waypointRadiusConnected = true;
connect(fact, &Fact::valueChanged, this, [this](QVariant value){
emit waypointRadiusChanged(value.toDouble());
emit showWaypointRadiusChanged();
});
}
test/MissionManager/SimpleMissionItemTest.h:31
- Typo in test slot name: “Propogation” → “Propagation”.
void _testAltitudePropogation();
void _testWaypointRadiusPropogation();
test/MissionManager/SimpleMissionItemTest.cc:334
- Typo in test function name: “Propogation” → “Propagation” (should match the header slot declaration).
void SimpleMissionItemTest::_testWaypointRadiusPropogation()
Contributor
|
See the Build Results workflow run for details. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce a Waypoint Radius Fact and UI/visual support.
Description
This pull request introduces a global Waypoint Radius setting and visual feedback in Plan Mode. Rather than being defined on a per-waypoint basis, the waypoint radius is managed as a global parameter that synchronizes bidirectionally with the flight controller's actual configuration (
WP_RADIUSfor ArduPilot Plane/Rover,WPNAV_RADIUSfor ArduPilot Copter, orNAV_ACC_RADfor PX4).Key Features:
Waypoint Radiuseditor field is always visible and editable in Plan view, even if no vehicle is connected (defaulting to 50m)..planJSON file and loaded back, fully preserving the mission plan setting across sessions.Type of Change
Testing
Platforms Tested
Flight Stacks Tested
Checklist
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).