From 7bc6bbb2c929f87e510de08db9db2b8a191bb3a0 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Fri, 10 Jul 2026 01:39:33 -0400 Subject: [PATCH 1/3] Keep event annotations attached to their node across tree mutations Post-refactor successor of fix/annotations, rebuilt for the shared sexp tree model introduced by the sexp_tree refactor (#7301). The refactor re-keyed annotations from UI item handles to tree_nodes[] indices, which fixed most of the original decoupling by construction and already prunes regular-node annotations in free_node2(). Two gaps remained, plus one qtFRED regression: - free_node2() only removed annotations keyed by node index, never a root-label annotation keyed rootKey(formula). Deleting an event left that annotation live, so a later event whose formula reused the slot inherited the deleted event's annotation. Remove the root key too; since the global Event_annotations are only rewritten at save, the removal still survives a Cancel. - Inserting an operator above a labeled root changes the event's formula node, orphaning the root-label annotation keyed to the old index. Re-key it in insert_operator() so it follows the event. (This is the only such flow: replace_operator() reuses the node in place.) - qtFRED reorderByRootFormulaOrder() reloaded annotations from the global state, discarding any annotation edits made since the dialog was opened. A root reorder changes neither node indices nor formulas, so drop the reload. Co-Authored-By: Claude Fable 5 --- code/missioneditor/sexp_tree_actions.cpp | 9 +++++++++ code/missioneditor/sexp_tree_model.cpp | 10 ++++++++-- .../src/mission/dialogs/MissionEventsDialogModel.cpp | 6 ++++-- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/code/missioneditor/sexp_tree_actions.cpp b/code/missioneditor/sexp_tree_actions.cpp index 52b85b61407..b5820663895 100644 --- a/code/missioneditor/sexp_tree_actions.cpp +++ b/code/missioneditor/sexp_tree_actions.cpp @@ -1,4 +1,5 @@ #include "missioneditor/sexp_tree_actions.h" +#include "missioneditor/sexp_annotation_model.h" #include "parse/sexp.h" #include "parse/sexp_container.h" @@ -506,6 +507,14 @@ int SexpTreeActions::insert_operator(int op, void* root_parent_handle) if (_model._interface && _model._interface->getFlags()[TreeFlags::LabeledRoot]) { parent_handle = root_parent_handle; _model._interface->onRootInserted(wrapped_node, node); + + // a root label's annotation is keyed to its formula node; re-key it + // so it follows the event across the formula change + if (_model.annotation_model) { + auto* ea = _model.annotation_model->getByKey(SexpAnnotationModel::rootKey(wrapped_node)); + if (ea) + ea->node_index = SexpAnnotationModel::rootKey(node); + } } else { _model.root_item = node; } diff --git a/code/missioneditor/sexp_tree_model.cpp b/code/missioneditor/sexp_tree_model.cpp index 25364bfb908..3af7a38e345 100644 --- a/code/missioneditor/sexp_tree_model.cpp +++ b/code/missioneditor/sexp_tree_model.cpp @@ -396,9 +396,15 @@ void SexpTreeModel::free_node2(int node) total_nodes--; // Remove any annotation referencing this node so that if allocate_node() - // reuses this slot, the new node won't inherit a stale annotation. - if (annotation_model) + // reuses this slot, the new node won't inherit a stale annotation. Also + // remove any root-label annotation keyed to this node as an event formula + // (rootKey), so that an event whose formula later lands in this slot won't + // inherit a deleted event's annotation. The global Event_annotations are + // only rewritten at save, so annotations removed here still survive a Cancel. + if (annotation_model) { annotation_model->removeByKey(node); + annotation_model->removeByKey(SexpAnnotationModel::rootKey(node)); + } // Recursively free children and following siblings. if (tree_nodes[node].child != -1) diff --git a/qtfred/src/mission/dialogs/MissionEventsDialogModel.cpp b/qtfred/src/mission/dialogs/MissionEventsDialogModel.cpp index 381ea486154..efd4c944533 100644 --- a/qtfred/src/mission/dialogs/MissionEventsDialogModel.cpp +++ b/qtfred/src/mission/dialogs/MissionEventsDialogModel.cpp @@ -486,8 +486,10 @@ void MissionEventsDialogModel::reorderByRootFormulaOrder(const SCP_vector& // Keep selection reasonable (select the first event after reorder) setCurrentlySelectedEvent(m_events.empty() ? -1 : 0); - // Rebuild applied annotations against new node indices/order if needed - initializeEventAnnotations(); + // Annotations are keyed by tree_nodes[] index (or by formula for root labels) + // and a root reorder changes neither, so the working annotation set is still + // valid. Do NOT reload it from the global state here (initializeEventAnnotations): + // that would discard any annotation edits made since the dialog was opened. set_modified(); } From e84c67c01c8083bc37e1393a2bee1e9fcdd23001 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Thu, 9 Jul 2026 23:56:23 -0400 Subject: [PATCH 2/3] Restore pre-refactor on-disk format for event annotation paths Annotation +Path: values written by pre-refactor FRED were built over the UI tree, where each event's root item is the event-name label and every element after the event index is a child position at successive levels below it: [event] is the label, [event, 0] the top operator (the label's only child), [event, 0, k...] its descendants. The refactored SexpAnnotationModel built paths over tree_nodes[], which represents the label's single child directly as the formula node, so paths came out one level shallower: existing missions' annotations resolved one level too deep (and were re-saved wrong), and a top-operator annotation produced the length-1 path that decodes as a root-label key, silently migrating onto the event label. Emit and consume the label-level child position in buildPath/resolveFromPath to preserve the on-disk contract. Child 0 resolves to the formula node and any other index fails resolution, matching the old traverse_path behavior of walking off the label's sibling list. Annotations on the event label itself are unaffected: they remain the length-1 path handled by the root-key branch. Co-Authored-By: Claude Fable 5 --- code/missioneditor/sexp_annotation_model.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/code/missioneditor/sexp_annotation_model.cpp b/code/missioneditor/sexp_annotation_model.cpp index 604ec403359..46b5ee148b0 100644 --- a/code/missioneditor/sexp_annotation_model.cpp +++ b/code/missioneditor/sexp_annotation_model.cpp @@ -202,6 +202,7 @@ SCP_list SexpAnnotationModel::buildPath(int key, const SCP_vector& path, const SCP_ve if (path.size() == 1) return rootKey(formula); - // Walk down the tree from the formula node. - int node = formula; auto it = path.begin(); ++it; // skip event index + + // A label has exactly one child; any other index fails. + if (*it != 0) + return -1; + ++it; + + // Walk down the tree from the formula node. + int node = formula; for (; it != path.end() && node >= 0; ++it) { int target = *it; if (node < 0 || node >= static_cast(tree_nodes.size())) From 5aabcccd112bb4a70b7ee9d4c563cba2d32ef7d1 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Fri, 10 Jul 2026 01:51:15 -0400 Subject: [PATCH 3/3] prevent zombie annotations and restore help box root-label comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root-label annotations (event-name comments/colors) that lose their node — e.g. because their event was deleted — could survive indefinitely and surface on the wrong items: - saveToGlobal's root-key branch kept an unresolvable annotation in Event_annotations with an empty path instead of marking it for pruning, so once a zombie was persisted in a mission file it survived every save/load cycle. - getByKey(-1) matched annotations whose node_index had been reset to the unresolved sentinel, so such a zombie's comment could display on any item that has no annotation key of its own. Guard findByKey against the -1 sentinel, and prune any annotation whose rebuilt path is empty at save time. (The complementary in-session fixes — removing a root-label annotation when its event is deleted and re-keying it when insert-operator changes the formula node — are handled in the shared model and actions by 'Keep event annotations attached to their node across tree mutations'.) Also restore root-label comments in the FRED2 help box: update_help resolved comments by model node index, which is -1 for labels. Route it through a get_item_comment virtual that the event editor's tree overrides with its annotation key lookup, matching pre-refactor behavior. (qtFRED renders comments per-item via NoteRole, which already covers labels; its help box does not show comments by design.) Co-Authored-By: Claude Fable 5 --- code/missioneditor/sexp_annotation_model.cpp | 17 +++++++++++++---- fred2/eventeditor.cpp | 7 +++++++ fred2/eventeditor.h | 1 + fred2/sexp_tree_view.cpp | 9 ++++++++- fred2/sexp_tree_view.h | 1 + 5 files changed, 30 insertions(+), 5 deletions(-) diff --git a/code/missioneditor/sexp_annotation_model.cpp b/code/missioneditor/sexp_annotation_model.cpp index 46b5ee148b0..3478642a817 100644 --- a/code/missioneditor/sexp_annotation_model.cpp +++ b/code/missioneditor/sexp_annotation_model.cpp @@ -55,13 +55,17 @@ void SexpAnnotationModel::saveToGlobal(const SCP_vector& tree_no int resolved = resolveFromPath(old_path, tree_nodes, events, sig); if (resolved >= 0 || isRootKey(resolved)) { ea.path = buildPath(resolved, tree_nodes, events); - } else { - // Truly gone; mark default for pruning. - ea.comment.clear(); - ea.r = ea.g = ea.b = 255; } } + // If no path could be built, the annotated node no longer exists (e.g. its + // event was deleted); mark default so prune() removes it rather than letting + // an unattachable annotation survive in the global list. + if (ea.path.empty()) { + ea.comment.clear(); + ea.r = ea.g = ea.b = 255; + } + // Reset transient field. ea.node_index = -1; } @@ -77,6 +81,11 @@ void SexpAnnotationModel::saveToGlobal(const SCP_vector& tree_no // Return the vector index of the annotation with the given key, or -1 if not found. int SexpAnnotationModel::findByKey(int key) const { + // -1 is the default/unresolved sentinel, not a real key; matching it would + // surface an unresolved annotation on any item that has no key of its own + if (key == -1) + return -1; + for (size_t i = 0; i < m_annotations.size(); ++i) { if (m_annotations[i].node_index == key) return static_cast(i); diff --git a/fred2/eventeditor.cpp b/fred2/eventeditor.cpp index 362bfc24df5..2308bfb2314 100644 --- a/fred2/eventeditor.cpp +++ b/fred2/eventeditor.cpp @@ -1918,3 +1918,10 @@ SCP_string event_sexp_tree::get_node_comment(int node_index) const return ""; } + +SCP_string event_sexp_tree::get_item_comment(HTREEITEM h, int /*node_index*/) +{ + // resolves both regular nodes and root labels (which have no model node, + // so the base class's node_index would be -1 for them) + return get_node_comment(annotation_key_for_item(this, h)); +} diff --git a/fred2/eventeditor.h b/fred2/eventeditor.h index ab6c1e3b3fb..6d329eb23db 100644 --- a/fred2/eventeditor.h +++ b/fred2/eventeditor.h @@ -28,6 +28,7 @@ class event_sexp_tree : public sexp_tree_view void edit_comment(HTREEITEM h); void edit_bg_color(HTREEITEM h); SCP_string get_node_comment(int node_index) const override; + SCP_string get_item_comment(HTREEITEM h, int node_index) override; // Set by event_editor during initialization SexpAnnotationModel* m_annotations = nullptr; diff --git a/fred2/sexp_tree_view.cpp b/fred2/sexp_tree_view.cpp index 8ebf2183ad6..b2f6f86fd88 100644 --- a/fred2/sexp_tree_view.cpp +++ b/fred2/sexp_tree_view.cpp @@ -617,6 +617,13 @@ SCP_string sexp_tree_view::get_node_comment(int /*node_index*/) const return ""; } +// Returns the comment for a tree item. The base class only has comments on model +// nodes; event_sexp_tree overrides this to also resolve labeled event roots. +SCP_string sexp_tree_view::get_item_comment(HTREEITEM /*h*/, int node_index) +{ + return get_node_comment(node_index); +} + // Handles completion of inline label editing. Validates the new text via // _model.validate_label_edit(), commits operator replacements or data changes, // and returns 0 if the edit was consumed, 1 if MFC should update the label. @@ -1759,7 +1766,7 @@ void sexp_tree_view::update_help(HTREEITEM h) // Build annotation comment SCP_string nodeComment; - SCP_string raw_comment = get_node_comment(node_index); + SCP_string raw_comment = get_item_comment(h, node_index); if (!raw_comment.empty()) { nodeComment = "Node Comments:\r\n " + raw_comment; } diff --git a/fred2/sexp_tree_view.h b/fred2/sexp_tree_view.h index 20f19cb611a..19facdde1d0 100644 --- a/fred2/sexp_tree_view.h +++ b/fred2/sexp_tree_view.h @@ -72,6 +72,7 @@ class sexp_tree_view : public CTreeCtrl, public ISexpTreeUI virtual void edit_comment(HTREEITEM h); virtual void edit_bg_color(HTREEITEM h); virtual SCP_string get_node_comment(int node_index) const; + virtual SCP_string get_item_comment(HTREEITEM h, int node_index); void right_clicked(); int ctree_size; virtual void build_tree();