From 435bb373faa299e1663e7bc344bc73b23542c9dc Mon Sep 17 00:00:00 2001
From: Federico Lolli <federico.lolli@skywarder.eu>
Date: Sun, 13 Apr 2025 17:23:07 +0200
Subject: [PATCH] fixed bug with removal of elements and connections related
crashes
---
src/ui/panes/pid_drawing_tool.rs | 72 ++++++++++++--------
src/ui/panes/pid_drawing_tool/connections.rs | 16 ++---
src/utils.rs | 1 +
src/utils/id.rs | 31 +++++++++
4 files changed, 84 insertions(+), 36 deletions(-)
create mode 100644 src/utils/id.rs
diff --git a/src/ui/panes/pid_drawing_tool.rs b/src/ui/panes/pid_drawing_tool.rs
index 9257cf4..3d4ed43 100644
--- a/src/ui/panes/pid_drawing_tool.rs
+++ b/src/ui/panes/pid_drawing_tool.rs
@@ -3,17 +3,14 @@ mod elements;
mod grid;
mod symbols;
-use connections::Connection;
use core::f32;
use egui::{
Button, Color32, Context, CursorIcon, PointerButton, Response, Sense, Theme, Ui, Widget,
};
-use elements::Element;
use glam::Vec2;
-use grid::GridInfo;
use serde::{Deserialize, Serialize};
+use std::collections::HashMap;
use strum::IntoEnumIterator;
-use symbols::{Symbol, icons::Icon};
use crate::{
MAVLINK_PROFILE,
@@ -22,15 +19,21 @@ use crate::{
ui::{
app::PaneResponse, cache::ChangeTracker, shortcuts::ShortcutHandler, utils::egui_to_glam,
},
+ utils::id::IdGenerator,
};
use super::PaneBehavior;
+use connections::Connection;
+use elements::Element;
+use grid::GridInfo;
+use symbols::{Symbol, icons::Icon};
+
#[derive(Clone, Debug)]
enum Action {
- Connect(usize),
+ Connect(u32),
ContextMenu(Vec2),
- DragElement(usize),
+ DragElement(u32),
DragConnection(usize, usize),
DragGrid,
}
@@ -39,7 +42,7 @@ enum Action {
#[derive(Clone, Serialize, Deserialize, Debug)]
pub struct PidPane {
// Persistent internal state
- elements: Vec<Element>,
+ elements: HashMap<u32, Element>,
connections: Vec<Connection>,
grid: GridInfo,
message_subscription_id: u32,
@@ -49,6 +52,8 @@ pub struct PidPane {
// Temporary internal state
#[serde(skip)]
+ id_generator: IdGenerator,
+ #[serde(skip)]
action: Option<Action>,
#[serde(skip)]
editable: bool,
@@ -59,11 +64,12 @@ pub struct PidPane {
impl Default for PidPane {
fn default() -> Self {
Self {
- elements: Vec::new(),
+ elements: HashMap::new(),
connections: Vec::new(),
grid: GridInfo::default(),
message_subscription_id: GSE_TM_DATA::ID,
center_content: false,
+ id_generator: IdGenerator::new(),
action: None,
editable: false,
is_subs_window_visible: false,
@@ -146,7 +152,7 @@ impl PaneBehavior for PidPane {
fn update(&mut self, messages: &[&TimedMessage]) {
if let Some(msg) = messages.last() {
- for element in &mut self.elements {
+ for element in self.elements.values_mut() {
element.update(&msg.message, self.message_subscription_id);
}
}
@@ -181,10 +187,11 @@ impl PidPane {
}
/// Returns the index of the element the point is on, if any
- fn hovers_element(&self, p_s: Vec2) -> Option<usize> {
+ fn hovers_element(&self, p_s: Vec2) -> Option<u32> {
self.elements
.iter()
- .position(|elem| elem.contains(self.grid.screen_to_grid(p_s)))
+ .find(|(_, elem)| elem.contains(self.grid.screen_to_grid(p_s)))
+ .map(|(idx, _)| *idx)
}
/// Return the connection and segment indexes where the position is on, if any
@@ -245,7 +252,7 @@ impl PidPane {
}
fn elements_ui(&mut self, ui: &mut Ui, theme: Theme) {
- for element in &mut self.elements {
+ for element in self.elements.values_mut() {
ui.scope(|ui| {
element.ui(ui, &self.grid, theme, self.message_subscription_id);
});
@@ -271,7 +278,10 @@ impl PidPane {
ui.close_menu();
}
let btn_response = Button::new("Delete").ui(ui);
- self.elements[elem_idx].context_menu(ui);
+ self.elements
+ .get_mut(&elem_idx)
+ .log_unwrap()
+ .context_menu(ui);
// Handle the delete button
if btn_response.clicked() {
self.delete_element(elem_idx);
@@ -287,14 +297,14 @@ impl PidPane {
if ui.button("Change start anchor").clicked() {
let conn = &mut self.connections[conn_idx];
conn.start_anchor =
- (conn.start_anchor + 1) % self.elements[conn.start].anchor_points_len();
+ (conn.start_anchor + 1) % self.elements[&conn.start].anchor_points_len();
self.action.take();
ui.close_menu();
}
if ui.button("Change end anchor").clicked() {
let conn = &mut self.connections[conn_idx];
conn.end_anchor =
- (conn.end_anchor + 1) % self.elements[conn.end].anchor_points_len();
+ (conn.end_anchor + 1) % self.elements[&conn.end].anchor_points_len();
self.action.take();
ui.close_menu();
}
@@ -305,20 +315,23 @@ impl PidPane {
ui.menu_button("Icons", |ui| {
for icon in Icon::iter() {
if ui.button(icon.to_string()).clicked() {
- self.elements.push(Element::new(
- self.grid.screen_to_grid(pointer_pos).round(),
- Symbol::Icon(icon),
- ));
+ self.elements.insert(
+ self.id_generator.next_id(),
+ Element::new(
+ self.grid.screen_to_grid(pointer_pos).round(),
+ Symbol::Icon(icon),
+ ),
+ );
self.action.take();
ui.close_menu();
}
}
});
} else if ui.button(symbol.to_string()).clicked() {
- self.elements.push(Element::new(
- self.grid.screen_to_grid(pointer_pos).round(),
- symbol,
- ));
+ self.elements.insert(
+ self.id_generator.next_id(),
+ Element::new(self.grid.screen_to_grid(pointer_pos).round(), symbol),
+ );
self.action.take();
ui.close_menu();
}
@@ -338,12 +351,12 @@ impl PidPane {
}
/// Removes an element from the diagram
- fn delete_element(&mut self, elem_idx: usize) {
+ fn delete_element(&mut self, elem_idx: u32) {
// First delete connection referencing this element
self.connections.retain(|elem| !elem.connected(elem_idx));
// Then the element
- self.elements.remove(elem_idx);
+ self.elements.remove(&elem_idx);
}
fn center(&mut self, ui: &Ui) {
@@ -352,7 +365,7 @@ impl PidPane {
// Chain elements positions and connection mid points
let points: Vec<Vec2> = self
.elements
- .iter()
+ .values()
.map(|e| e.center())
.chain(self.connections.iter().flat_map(|conn| conn.points()))
.collect();
@@ -436,7 +449,10 @@ impl PidPane {
}
Some(Action::DragElement(idx)) => {
let pointer_pos_g = self.grid.screen_to_grid(pointer_pos).round();
- self.elements[idx].set_center_at(pointer_pos_g);
+ self.elements
+ .get_mut(&idx)
+ .log_unwrap()
+ .set_center_at(pointer_pos_g);
}
Some(Action::DragConnection(conn_idx, point_idx)) => {
let pointer_pos_g = self.grid.screen_to_grid(pointer_pos).round();
@@ -452,7 +468,7 @@ impl PidPane {
}
fn reset_subscriptions(&mut self) {
- for element in &mut self.elements {
+ for element in self.elements.values_mut() {
element.reset_subscriptions();
}
}
diff --git a/src/ui/panes/pid_drawing_tool/connections.rs b/src/ui/panes/pid_drawing_tool/connections.rs
index 3cde5f8..942984f 100644
--- a/src/ui/panes/pid_drawing_tool/connections.rs
+++ b/src/ui/panes/pid_drawing_tool/connections.rs
@@ -12,11 +12,11 @@ use super::{
#[derive(Clone, Serialize, Deserialize, PartialEq, Debug)]
pub struct Connection {
/// Index of the start element
- pub start: usize,
+ pub start: u32,
pub start_anchor: usize,
/// Index of the end element
- pub end: usize,
+ pub end: u32,
pub end_anchor: usize,
/// Mid points in grid coordinates
@@ -24,7 +24,7 @@ pub struct Connection {
}
impl Connection {
- pub fn new(start: usize, start_anchor: usize, end: usize, end_anchor: usize) -> Self {
+ pub fn new(start: u32, start_anchor: usize, end: u32, end_anchor: usize) -> Self {
Self {
start,
start_anchor,
@@ -45,13 +45,13 @@ impl Connection {
let mut points = Vec::new();
// Append start point
- points.push(pid.elements[self.start].anchor_point(self.start_anchor));
+ points.push(pid.elements[&self.start].anchor_point(self.start_anchor));
// Append all midpoints
self.points_g.iter().for_each(|&p| points.push(p));
// Append end point
- points.push(pid.elements[self.end].anchor_point(self.end_anchor));
+ points.push(pid.elements[&self.end].anchor_point(self.end_anchor));
// Check each segment
for i in 0..(points.len() - 1) {
@@ -66,7 +66,7 @@ impl Connection {
}
/// Checks if the connection references the given element index
- pub fn connected(&self, elem_idx: usize) -> bool {
+ pub fn connected(&self, elem_idx: u32) -> bool {
self.start == elem_idx || self.end == elem_idx
}
@@ -97,9 +97,9 @@ impl Connection {
pub fn draw(&self, pid: &PidPane, painter: &Painter, theme: Theme) {
let color = Connection::line_color(theme);
- let start = pid.elements[self.start].anchor_point(self.start_anchor);
+ let start = pid.elements[&self.start].anchor_point(self.start_anchor);
let start = pid.grid.grid_to_screen(start);
- let end = pid.elements[self.end].anchor_point(self.end_anchor);
+ let end = pid.elements[&self.end].anchor_point(self.end_anchor);
let end = pid.grid.grid_to_screen(end);
// Draw line segments
diff --git a/src/utils.rs b/src/utils.rs
index 503a473..3ff5656 100644
--- a/src/utils.rs
+++ b/src/utils.rs
@@ -1,2 +1,3 @@
+pub mod id;
mod ring_buffer;
pub mod units;
diff --git a/src/utils/id.rs b/src/utils/id.rs
new file mode 100644
index 0000000..04b06ea
--- /dev/null
+++ b/src/utils/id.rs
@@ -0,0 +1,31 @@
+use std::collections::HashSet;
+
+/// A simple id generator that wraps around when it reaches the maximum value.
+#[derive(Debug, Clone, Default)]
+pub struct IdGenerator {
+ current: u32,
+ already_used: HashSet<u32>,
+}
+
+impl IdGenerator {
+ pub fn new() -> Self {
+ Self::default()
+ }
+
+ /// Get the next id, wrapping around if necessary.
+ pub fn next_id(&mut self) -> u32 {
+ loop {
+ self.current = self.current.wrapping_add(1);
+ if !self.already_used.contains(&self.current) {
+ self.already_used.insert(self.current);
+ return self.current;
+ }
+ }
+ }
+
+ /// Release an id, making it available for reuse. Do not change the current
+ /// id to avoid useless checks and exploit wrapping increment.
+ pub fn release_id(&mut self, id: u32) {
+ self.already_used.remove(&id);
+ }
+}
--
GitLab