From a644d94ed219d43eae04af95660b99659961e257 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Mon, 30 Mar 2026 02:36:33 -0400 Subject: [PATCH 1/2] Remove redundant m_pos from CJumpNode; add GetRadius() accessor CJumpNode stored a private m_pos field that duplicated the position already held by its Objects[] entry. The field was set once during construction and never updated afterward, so any subsequent movement (e.g. FRED dragging) would leave m_pos stale while Objects[m_objnum].pos held the authoritative value. Every external caller already used GetSCPObject()->pos instead; GetPosition() had zero call sites. - Remove the m_pos field and its initialization in both constructors, the move constructor, and the move-assignment operator. - Pass the constructor's position argument directly to obj_create() instead of routing through m_pos. - Repoint GetPosition() at Objects[m_objnum].pos so it always returns the live, up-to-date position. - Add GetRadius() to expose the cached m_radius, and use it in jumpnode_get_which_in() instead of the redundant model_get_radius() call. Co-Authored-By: Claude Opus 4.6 (1M context) --- code/jumpnode/jumpnode.cpp | 45 +++++++++++++++++++------------------- code/jumpnode/jumpnode.h | 2 +- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/code/jumpnode/jumpnode.cpp b/code/jumpnode/jumpnode.cpp index 83f11e80055..351640e179f 100644 --- a/code/jumpnode/jumpnode.cpp +++ b/code/jumpnode/jumpnode.cpp @@ -19,30 +19,28 @@ SCP_list Jump_nodes; * Constructor for CJumpNode class, default */ CJumpNode::CJumpNode() -{ +{ gr_init_alphacolor(&m_display_color, 0, 255, 0, 255); m_name[0] = '\0'; m_display[0] = '\0'; - - m_pos.xyz.x = 0.0f; - m_pos.xyz.y = 0.0f; - m_pos.xyz.z = 0.0f; } /** * Constructor for CJumpNode class, with world position argument */ CJumpNode::CJumpNode(const vec3d* position) -{ - Assert(position != NULL); - +{ + Assertion(position != nullptr, "Position should not be null!"); + if (position == nullptr) + position = &vmd_zero_vector; + gr_init_alphacolor(&m_display_color, 0, 255, 0, 255); - + // Set m_name and m_display sprintf(m_name, XSTR( "Jump Node %d", 632), Jump_nodes.size()); m_display[0] = '\0'; - + // Set m_modelnum and m_radius m_modelnum = model_load(NOX(JN_DEFAULT_MODEL), nullptr, ErrorType::WARNING); if (m_modelnum == -1) { @@ -50,15 +48,11 @@ CJumpNode::CJumpNode(const vec3d* position) } else { m_radius = model_get_radius(m_modelnum); } - - m_pos.xyz.x = position->xyz.x; - m_pos.xyz.y = position->xyz.y; - m_pos.xyz.z = position->xyz.z; - + // Create the object - flagset default_flags; - default_flags.set(Object::Object_Flags::Renders); - m_objnum = obj_create(OBJ_JUMP_NODE, -1, -1, NULL, &m_pos, m_radius, default_flags); + flagset default_flags; + default_flags.set(Object::Object_Flags::Renders); + m_objnum = obj_create(OBJ_JUMP_NODE, -1, -1, nullptr, position, m_radius, default_flags); if (m_modelnum >= 0) { // set up animation in case of instrinsic_rotate @@ -81,7 +75,6 @@ CJumpNode::CJumpNode(CJumpNode&& other) noexcept other.m_fred_layer = "Default"; m_display_color = other.m_display_color; - m_pos = other.m_pos; strcpy_s(m_name, other.m_name); strcpy_s(m_display, other.m_display); @@ -106,7 +99,6 @@ CJumpNode& CJumpNode::operator=(CJumpNode&& other) noexcept other.m_fred_layer = "Default"; m_display_color = other.m_display_color; - m_pos = other.m_pos; strcpy_s(m_name, other.m_name); strcpy_s(m_display, other.m_display); @@ -160,6 +152,14 @@ int CJumpNode::GetModelNumber() const return m_modelnum; } +/** + * @return Radius of jump node model + */ +float CJumpNode::GetRadius() const +{ + return m_radius; +} + /** * @return Index into Objects[] */ @@ -190,7 +190,8 @@ const color &CJumpNode::GetColor() const */ const vec3d *CJumpNode::GetPosition() const { - return &m_pos; + Assert(m_objnum != -1); + return &Objects[m_objnum].pos; } /* @@ -536,7 +537,7 @@ CJumpNode *jumpnode_get_which_in(const object *objp) if(jnp->GetModelNumber() < 0) continue; - radius = model_get_radius( jnp->GetModelNumber() ); + radius = jnp->GetRadius(); dist = vm_vec_dist( &objp->pos, &jnp->GetSCPObject()->pos ); if ( dist <= radius ) { return &(*jnp); diff --git a/code/jumpnode/jumpnode.h b/code/jumpnode/jumpnode.h index 37ed84686af..05fadd320a9 100644 --- a/code/jumpnode/jumpnode.h +++ b/code/jumpnode/jumpnode.h @@ -45,7 +45,6 @@ class CJumpNode int m_flags {0}; color m_display_color; // Color node will be shown in (Default:0/255/0/255) - vec3d m_pos; SCP_string m_fred_layer = "Default"; // FRED view layer assignment CJumpNode(const CJumpNode&); @@ -65,6 +64,7 @@ class CJumpNode const char *GetName() const; const char *GetDisplayName() const; int GetModelNumber() const; + float GetRadius() const; int GetSCPObjectNumber() const; int GetPolymodelInstanceNum() const; const object *GetSCPObject() const; From 543f1ef69e431e1246c20aa8fad7a776b71f96c9 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Mon, 30 Mar 2026 17:18:03 -0400 Subject: [PATCH 2/2] refactor CJumpNode to use SCP_vector Jump nodes previously used SCP_list; refactor to use SCP_vector. Follow-up to #7431. --- code/hud/hudtargetbox.cpp | 14 ++++----- code/jumpnode/jumpnode.cpp | 44 ++++++++++++++++++----------- code/jumpnode/jumpnode.h | 3 +- code/missioneditor/missionsave.cpp | 29 +++++++++---------- code/object/object.cpp | 6 ++-- fred2/dumpstats.cpp | 5 ++-- fred2/jumpnodedlg.cpp | 10 +++---- fred2/management.cpp | 10 +++---- fred2/sexp_tree.cpp | 5 ++-- qtfred/src/mission/Editor.cpp | 9 ++---- qtfred/src/ui/widgets/sexp_tree.cpp | 5 ++-- 11 files changed, 70 insertions(+), 70 deletions(-) diff --git a/code/hud/hudtargetbox.cpp b/code/hud/hudtargetbox.cpp index fb1237cecf1..2edd689179d 100644 --- a/code/hud/hudtargetbox.cpp +++ b/code/hud/hudtargetbox.cpp @@ -1322,13 +1322,11 @@ void HudGaugeTargetBox::renderTargetJumpNode(object *target_objp) matrix camera_orient = IDENTITY_MATRIX; float factor, dist; int hx = 0, hy = 0, w, h; - SCP_list::iterator jnp; - - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - if(jnp->GetSCPObject() != target_objp) + for (auto &jnp : Jump_nodes) { + if(jnp.GetSCPObject() != target_objp) continue; - - if ( jnp->IsHidden() ) { + + if ( jnp.IsHidden() ) { set_target_objnum( Player_ai, -1 ); return; } @@ -1357,7 +1355,7 @@ void HudGaugeTargetBox::renderTargetJumpNode(object *target_objp) gr_stencil_set(GR_STENCIL_READ); } - jnp->Render( &obj_pos ); + jnp.Render( &obj_pos ); if ( Monitor_mask >= 0 ) { gr_stencil_set(GR_STENCIL_NONE); @@ -1370,7 +1368,7 @@ void HudGaugeTargetBox::renderTargetJumpNode(object *target_objp) renderTargetIntegrity(1); setGaugeColor(); - renderString(position[0] + Name_offsets[0], position[1] + Name_offsets[1], EG_TBOX_NAME, jnp->GetDisplayName()); + renderString(position[0] + Name_offsets[0], position[1] + Name_offsets[1], EG_TBOX_NAME, jnp.GetDisplayName()); dist = Player_ai->current_target_distance; if ( Hud_unit_multiplier > 0.0f ) { // use a different displayed distance scale diff --git a/code/jumpnode/jumpnode.cpp b/code/jumpnode/jumpnode.cpp index 351640e179f..c60a63f1752 100644 --- a/code/jumpnode/jumpnode.cpp +++ b/code/jumpnode/jumpnode.cpp @@ -13,7 +13,7 @@ #include "model/model.h" #include "model/modelrender.h" -SCP_list Jump_nodes; +SCP_vector Jump_nodes; /** * Constructor for CJumpNode class, default @@ -475,16 +475,32 @@ void CJumpNode::Render(model_draw_list *scene, const vec3d *pos, const vec3d *vi CJumpNode *jumpnode_get_by_name(const char* name) { Assert(name != NULL); - SCP_list::iterator jnp; - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - if(!stricmp(jnp->GetName(), name)) - return &(*jnp); + for (auto &jnp : Jump_nodes) { + if(!stricmp(jnp.GetName(), name)) + return &jnp; } return NULL; } +/** + * Get jump node index by given name + * + * @param name Name of jump node + * @return Jump node index + */ +int jumpnode_lookup(const char *name) +{ + Assert(name != nullptr); + + for (size_t i = 0; i < Jump_nodes.size(); i++) + if (!stricmp(Jump_nodes[i].GetName(), name)) + return sz2i(i); + + return -1; +} + /** * Get jump node object by the object number * @@ -530,17 +546,15 @@ CJumpNode *jumpnode_get_by_objp(const object *objp) CJumpNode *jumpnode_get_which_in(const object *objp) { Assert(objp != NULL); - SCP_list::iterator jnp; - float radius, dist; - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - if(jnp->GetModelNumber() < 0) + for (auto &jnp : Jump_nodes) { + if(jnp.GetModelNumber() < 0) continue; - radius = jnp->GetRadius(); - dist = vm_vec_dist( &objp->pos, &jnp->GetSCPObject()->pos ); + float radius = jnp.GetRadius(); + float dist = vm_vec_dist( &objp->pos, &jnp.GetSCPObject()->pos ); if ( dist <= radius ) { - return &(*jnp); + return &jnp; } } @@ -554,10 +568,8 @@ CJumpNode *jumpnode_get_which_in(const object *objp) */ void jumpnode_render_all() { - SCP_list::iterator jnp; - - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - jnp->Render(&jnp->GetSCPObject()->pos); + for (auto &jnp : Jump_nodes) { + jnp.Render(&jnp.GetSCPObject()->pos); } } diff --git a/code/jumpnode/jumpnode.h b/code/jumpnode/jumpnode.h index 05fadd320a9..6479fdaa845 100644 --- a/code/jumpnode/jumpnode.h +++ b/code/jumpnode/jumpnode.h @@ -94,10 +94,11 @@ class CJumpNode }; //-----Globals------ -extern SCP_list Jump_nodes; +extern SCP_vector Jump_nodes; //-----Functions----- CJumpNode *jumpnode_get_by_name(const char *name); + int jumpnode_lookup(const char *name); CJumpNode *jumpnode_get_by_objnum(int objnum); CJumpNode *jumpnode_get_by_objp(const object *objp); CJumpNode *jumpnode_get_which_in(const object *objp); diff --git a/code/missioneditor/missionsave.cpp b/code/missioneditor/missionsave.cpp index 505163dee6c..f0e8106fee2 100644 --- a/code/missioneditor/missionsave.cpp +++ b/code/missioneditor/missionsave.cpp @@ -4791,56 +4791,55 @@ int Fred_mission_save::save_waypoints() parse_comments(2); fout("\t\t;! %d lists total\n", Waypoint_lists.size()); - SCP_list::iterator jnp; - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { + for (auto &jn : Jump_nodes) { required_string_fred("$Jump Node:", "$Jump Node Name:"); parse_comments(2); - save_vector(jnp->GetSCPObject()->pos); + save_vector(jn.GetSCPObject()->pos); required_string_fred("$Jump Node Name:", "$Jump Node:"); parse_comments(); - fout(" %s", jnp->GetName()); + fout(" %s", jn.GetName()); if (save_config.save_format != MissionFormat::RETAIL) { // The display name is only written if there was one at the start to avoid introducing inconsistencies - if (save_config.always_save_display_names || jnp->HasDisplayName()) { + if (save_config.always_save_display_names || jn.HasDisplayName()) { char truncated_name[NAME_LENGTH]; - strcpy_s(truncated_name, jnp->GetName()); + strcpy_s(truncated_name, jn.GetName()); end_string_at_first_hash_symbol(truncated_name); // Also, the display name is not written if it's just the truncation of the name at the hash - if (save_config.always_save_display_names || strcmp(jnp->GetDisplayName(), truncated_name) != 0) { + if (save_config.always_save_display_names || strcmp(jn.GetDisplayName(), truncated_name) != 0) { if (optional_string_fred("+Display Name:", "$Jump Node:")) { parse_comments(); } else { fout("\n+Display Name:"); } - fout_ext("", "%s", jnp->GetDisplayName()); + fout_ext("", "%s", jn.GetDisplayName()); } } - if (jnp->IsSpecialModel()) { + if (jn.IsSpecialModel()) { if (optional_string_fred("+Model File:", "$Jump Node:")) { parse_comments(); } else { fout("\n+Model File:"); } - int model = jnp->GetModelNumber(); + int model = jn.GetModelNumber(); polymodel* pm = model_get(model); fout(" %s", pm->filename); } - if (jnp->IsColored()) { + if (jn.IsColored()) { if (optional_string_fred("+Alphacolor:", "$Jump Node:")) { parse_comments(); } else { fout("\n+Alphacolor:"); } - const auto& jn_color = jnp->GetColor(); + const auto& jn_color = jn.GetColor(); fout(" %u %u %u %u", jn_color.red, jn_color.green, jn_color.blue, jn_color.alpha); } @@ -4848,17 +4847,17 @@ int Fred_mission_save::save_waypoints() if (hidden_is_there) parse_comments(); - if (hidden_is_there || jnp->IsHidden()) { + if (hidden_is_there || jn.IsHidden()) { if (!hidden_is_there) fout("\n+Hidden:"); - if (jnp->IsHidden()) + if (jn.IsHidden()) fout(" %s", "true"); else fout(" %s", "false"); } - const SCP_string& jn_layer = jnp->GetFredLayer(); + const SCP_string& jn_layer = jn.GetFredLayer(); if (!jn_layer.empty() && !lcase_equal(jn_layer, "Default")) { if (optional_string_fred("+Layer:", "$Jump Node:")) parse_comments(); diff --git a/code/object/object.cpp b/code/object/object.cpp index 06bbf7cb240..40530fb5394 100644 --- a/code/object/object.cpp +++ b/code/object/object.cpp @@ -1941,12 +1941,12 @@ void obj_queue_render(object* obj, model_draw_list* scene) asteroid_render(obj, scene); break; case OBJ_JUMP_NODE: - for ( SCP_list::iterator jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp ) { - if ( jnp->GetSCPObject() != obj ) { + for ( auto &jnp : Jump_nodes ) { + if ( jnp.GetSCPObject() != obj ) { continue; } - jnp->Render(scene, &obj->pos, &Eye_position); + jnp.Render(scene, &obj->pos, &Eye_position); } break; case OBJ_WAYPOINT: diff --git a/fred2/dumpstats.cpp b/fred2/dumpstats.cpp index 3959c9901ea..e087a7b6d2d 100644 --- a/fred2/dumpstats.cpp +++ b/fred2/dumpstats.cpp @@ -395,9 +395,8 @@ void DumpStats::get_object_stats(CString &buffer) // Jumpnodes buffer += "\r\nJUMPNODES\r\n"; - SCP_list::iterator jnp; - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - temp.Format("\tJumpnode: %s\r\n", jnp->GetName()); + for (auto &jnp : Jump_nodes) { + temp.Format("\tJumpnode: %s\r\n", jnp.GetName()); buffer += temp; } diff --git a/fred2/jumpnodedlg.cpp b/fred2/jumpnodedlg.cpp index 45c3400385e..ef4c916d2b3 100644 --- a/fred2/jumpnodedlg.cpp +++ b/fred2/jumpnodedlg.cpp @@ -86,20 +86,18 @@ BOOL jumpnode_dlg::Create() void jumpnode_dlg::OnInitMenu(CMenu* pMenu) { int i; - SCP_list::iterator jnp; CMenu *m; m = pMenu->GetSubMenu(0); clear_menu(m); - i = 0; - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - m->AppendMenu(MF_ENABLED | MF_STRING, ID_JUMP_NODE_MENU + i, jnp->GetName()); - if (jnp->GetSCPObjectNumber() == cur_object_index) { + i = 0; + for (auto &jnp : Jump_nodes) { + m->AppendMenu(MF_ENABLED | MF_STRING, ID_JUMP_NODE_MENU + i, jnp.GetName()); + if (jnp.GetSCPObjectNumber() == cur_object_index) { m->CheckMenuItem(ID_JUMP_NODE_MENU + i, MF_BYCOMMAND | MF_CHECKED); } i++; - } m->DeleteMenu(ID_PLACEHOLDER, MF_BYCOMMAND); diff --git a/fred2/management.cpp b/fred2/management.cpp index 5021609a630..58b49382a4f 100644 --- a/fred2/management.cpp +++ b/fred2/management.cpp @@ -10,6 +10,7 @@ #include "stdafx.h" +#include #include "FRED.h" #include "MainFrm.h" #include "FREDDoc.h" @@ -1229,7 +1230,6 @@ int common_object_delete(int obj) const char *name; int i, z, r, type; object *objp; - SCP_list::iterator jnp; type = Objects[obj].type; if (type == OBJ_START) { @@ -1365,15 +1365,13 @@ int common_object_delete(int obj) return 0; } else if (type == OBJ_JUMP_NODE) { - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - if(jnp->GetSCPObject() == &Objects[obj]) - break; - } + auto jnp = std::find_if(Jump_nodes.begin(), Jump_nodes.end(), + [obj](const CJumpNode &jn) { return jn.GetSCPObject() == &Objects[obj]; }); // come on, WMC, we don't want to call obj_delete twice... // fool the destructor into not calling obj_delete yet Objects[obj].type = OBJ_NONE; - + // now call the destructor if (jnp != Jump_nodes.end()) Jump_nodes.erase(jnp); diff --git a/fred2/sexp_tree.cpp b/fred2/sexp_tree.cpp index a37a1d03cdf..81ff933d2b2 100644 --- a/fred2/sexp_tree.cpp +++ b/fred2/sexp_tree.cpp @@ -7396,9 +7396,8 @@ sexp_list_item *sexp_tree::get_listing_opf_jump_nodes() { sexp_list_item head; - SCP_list::iterator jnp; - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - head.add_data( jnp->GetName()); + for (auto &jnp : Jump_nodes) { + head.add_data( jnp.GetName()); } return head.next; diff --git a/qtfred/src/mission/Editor.cpp b/qtfred/src/mission/Editor.cpp index 2f148873679..79f3bcb6ca6 100644 --- a/qtfred/src/mission/Editor.cpp +++ b/qtfred/src/mission/Editor.cpp @@ -1,5 +1,6 @@ #include "Editor.h" +#include #include #include #include @@ -1062,7 +1063,6 @@ int Editor::common_object_delete(int obj) { const char *name; int i, z, r, type; object* objp; - SCP_list::iterator jnp; type = Objects[obj].type; if (type == OBJ_START) { @@ -1209,11 +1209,8 @@ int Editor::common_object_delete(int obj) { return 0; } else if (type == OBJ_JUMP_NODE) { - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - if (jnp->GetSCPObject() == &Objects[obj]) { - break; - } - } + auto jnp = std::find_if(Jump_nodes.begin(), Jump_nodes.end(), + [obj](const CJumpNode &jn) { return jn.GetSCPObject() == &Objects[obj]; }); // come on, WMC, we don't want to call obj_delete twice... // fool the destructor into not calling obj_delete yet diff --git a/qtfred/src/ui/widgets/sexp_tree.cpp b/qtfred/src/ui/widgets/sexp_tree.cpp index c4e0d23e7f8..46f6b0584d4 100644 --- a/qtfred/src/ui/widgets/sexp_tree.cpp +++ b/qtfred/src/ui/widgets/sexp_tree.cpp @@ -5267,9 +5267,8 @@ sexp_list_item* sexp_tree::get_listing_opf_subsys_or_generic(int parent_node, in sexp_list_item* sexp_tree::get_listing_opf_jump_nodes() { sexp_list_item head; - SCP_list::iterator jnp; - for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) { - head.add_data(jnp->GetName()); + for (auto &jnp : Jump_nodes) { + head.add_data(jnp.GetName()); } return head.next;