Qtfred error checker (scp-fs2open#7389)

* new qtfred error checker

* fix some issues

* clang

* more clang

* clean up and fixes

* missed a comment

* add help documentation

* surface auto corrected issues more clearly

* some cleanup

Author: MjnMixael

code/globalincs/pstypes.h

@@ -364,6 +364,7 @@ const float PI_4		= (PI/4.0f);
 
 
 extern int Fred_running;  // Is Fred running, or FreeSpace?
+extern int Qtfred_running;  // Distinguishes QtFRED from legacy Fred2; Fred_running is set in both, but Qtfred_running only in QtFRED.
 extern bool running_unittests;
 
 const size_t INVALID_SIZE = static_cast<size_t>(-1);

code/mission/missionparse.cpp

@@ -175,6 +175,27 @@ p_object *Player_start_pobject;
 // something before that ship has even been loaded yet)
 SCP_vector<SCP_string> Parse_names;
 
+SCP_vector<SCP_string> Mission_parse_warnings;
+
+// Routes a parse-time auto-correction notice to the right surface for the app:
+// outside QtFRED, the existing Warning(LOCATION, ...) popup; inside QtFRED, the
+// Mission_parse_warnings queue so the ErrorChecker can present it without a popup.
+static void parse_warning_or_record(SCP_FORMAT_STRING const char* fmt, ...) SCP_FORMAT_STRING_ARGS(1, 2);
+static void parse_warning_or_record(const char* fmt, ...)
+{
+	SCP_string msg;
+	va_list args;
+	va_start(args, fmt);
+	vsprintf(msg, fmt, args);
+	va_end(args);
+
+	if (Qtfred_running) {
+		Mission_parse_warnings.push_back(std::move(msg));
+	} else {
+		Warning(LOCATION, "%s", msg.c_str());
+	}
+}
+
 SCP_vector<texture_replace> Fred_texture_replacements;
 
 SCP_unordered_set<int> Fred_migrated_immobile_ships;
@@ -2426,10 +2447,10 @@ int parse_create_object_sub(p_object *p_objp, bool standalone_ship)
 		// will accept were apparently written out incorrectly with Fred.  This Int3() should
 		// trap these instances.
 #ifndef NDEBUG
-		if (Fred_running)
+		if (Fred_running && !Qtfred_running)
 		{
 			std::set<size_t> default_orders, remaining_orders;
-			
+
 			default_orders = ship_get_default_orders_accepted(&Ship_info[shipp->ship_info_index]);
 			std::set_difference(p_objp->orders_accepted.begin(), p_objp->orders_accepted.end(), default_orders.begin(), default_orders.end(),
 								  std::inserter(remaining_orders, remaining_orders.begin()));
@@ -2963,7 +2984,7 @@ void resolve_parse_flags(object *objp, flagset<Mission::Parse_Object_Flags> &par
 
     if ((parse_flags[Mission::Parse_Object_Flags::OF_No_shields]) && (parse_flags[Mission::Parse_Object_Flags::OF_Force_shields_on]))
     {
-        Warning(LOCATION, "The parser found a ship with both the \"force-shields-on\" and \"no-shields\" flags; this is inconsistent!");
+        parse_warning_or_record("Ship %s has both the \"force-shields-on\" and \"no-shields\" flags; this is inconsistent.", shipp->ship_name);
     }
     if (parse_flags[Mission::Parse_Object_Flags::OF_No_shields])
         objp->flags.set(Object::Object_Flags::No_shields);
@@ -3458,7 +3479,7 @@ int parse_object(mission *pm, int  /*flag*/, p_object *p_objp)
 			|| (p_objp->arrival_location == ArrivalLocation::ABOVE_SHIP) || (p_objp->arrival_location == ArrivalLocation::BELOW_SHIP)
 			|| (p_objp->arrival_location == ArrivalLocation::TO_LEFT_OF_SHIP) || (p_objp->arrival_location == ArrivalLocation::TO_RIGHT_OF_SHIP) ))
 		{
-			Warning(LOCATION, "Arrival distance for ship %s cannot be %d.  Setting to 1.\n", p_objp->name, p_objp->arrival_distance);
+			parse_warning_or_record("Arrival distance for ship %s cannot be %d — corrected to 1.", p_objp->name, p_objp->arrival_distance);
 			p_objp->arrival_distance = 1;
 		}
 	}
@@ -3483,7 +3504,7 @@ int parse_object(mission *pm, int  /*flag*/, p_object *p_objp)
 		stuff_int(&delay);
 		if (delay < 0)
 		{
-			Warning(LOCATION, "Cannot have arrival delay < 0 on ship %s", p_objp->name);
+			parse_warning_or_record("Arrival delay on ship %s cannot be negative — corrected to 0.", p_objp->name);
 			delay = 0;
 		}
 
@@ -3520,7 +3541,7 @@ int parse_object(mission *pm, int  /*flag*/, p_object *p_objp)
 		stuff_int(&delay);
 		if (delay < 0)
 		{
-			Warning(LOCATION, "Cannot have departure delay < 0 (ship %s)", p_objp->name);
+			parse_warning_or_record("Departure delay on ship %s cannot be negative — corrected to 0.", p_objp->name);
 			delay = 0;
 		}
 
@@ -3752,7 +3773,7 @@ int parse_object(mission *pm, int  /*flag*/, p_object *p_objp)
 		stuff_int(&p_objp->destroy_before_mission_time);
 		if (p_objp->destroy_before_mission_time < 0)
 		{
-			Warning(LOCATION, "Cannot set a negative 'destroy before mission' value (ship %s)", p_objp->name);
+			parse_warning_or_record("'Destroy before mission' value on ship %s cannot be negative — corrected to 0.", p_objp->name);
 			p_objp->destroy_before_mission_time = 0;
 		}
 
@@ -3847,7 +3868,7 @@ int parse_object(mission *pm, int  /*flag*/, p_object *p_objp)
 	if (optional_string("+Persona Index:")) {
 		stuff_int(&p_objp->persona_index);
 		if (p_objp->persona_index < -1 || p_objp->persona_index >= (int)Personas.size()) {
-			Warning(LOCATION, "Persona index %d for %s is out of range!  Setting to -1.", p_objp->persona_index, p_objp->name);
+			parse_warning_or_record("Persona index %d for ship %s is out of range — corrected to -1.", p_objp->persona_index, p_objp->name);
 			p_objp->persona_index = -1;
 		}
 	}
@@ -4915,7 +4936,7 @@ void parse_wing(mission *pm)
 			|| (wingp->arrival_location == ArrivalLocation::ABOVE_SHIP) || (wingp->arrival_location == ArrivalLocation::BELOW_SHIP)
 			|| (wingp->arrival_location == ArrivalLocation::TO_LEFT_OF_SHIP) || (wingp->arrival_location == ArrivalLocation::TO_RIGHT_OF_SHIP) ))
 		{
-			Warning(LOCATION, "Arrival distance for wing %s cannot be %d.  Setting to 1.\n", wingp->name, wingp->arrival_distance);
+			parse_warning_or_record("Arrival distance for wing %s cannot be %d — corrected to 1.", wingp->name, wingp->arrival_distance);
 			wingp->arrival_distance = 1;
 		}
 	}
@@ -4940,7 +4961,7 @@ void parse_wing(mission *pm)
 		stuff_int(&delay);
 		if (delay < 0)
 		{
-			Warning(LOCATION, "Cannot have arrival delay < 0 on wing %s", wingp->name);
+			parse_warning_or_record("Arrival delay on wing %s cannot be negative — corrected to 0.", wingp->name);
 			delay = 0;
 		}
 
@@ -4977,7 +4998,7 @@ void parse_wing(mission *pm)
 		stuff_int(&delay);
 		if (delay < 0)
 		{
-			Warning(LOCATION, "Cannot have departure delay < 0 on wing %s", wingp->name);
+			parse_warning_or_record("Departure delay on wing %s cannot be negative — corrected to 0.", wingp->name);
 			delay = 0;
 		}
 
@@ -5152,7 +5173,7 @@ void parse_wing(mission *pm)
 
 			// Goober5000 - if this is a player start object, there shouldn't be a wing arrival delay (Mantis #2678)
 			if ((p_objp->flags[Mission::Parse_Object_Flags::OF_Player_start]) && (wingp->arrival_delay != 0)) {
-				Warning(LOCATION, "Wing %s specifies an arrival delay of %ds, but it also contains a player.  The arrival delay will be reset to 0.", wingp->name, abs(wingp->arrival_delay));
+				parse_warning_or_record("Wing %s specifies an arrival delay of %ds, but it also contains a player — corrected to 0.", wingp->name, abs(wingp->arrival_delay));
 				if (!Fred_running && wingp->arrival_delay > 0) {
 					// timestamp has been set, so set it again
 					wingp->arrival_delay = timestamp(0);
@@ -5423,7 +5444,9 @@ void post_process_ships_wings()
 	// error checking for custom wings
 	if (strcmp(Starting_wing_names[0], TVT_wing_names[0]) != 0)
 	{
-		Error(LOCATION, "The first starting wing and the first team-versus-team wing must have the same wing name.\n");
+		// In QtFRED this is surfaced via ErrorChecker::checkPlayerWings so the editor can load the mission.
+		if (!Qtfred_running)
+			Error(LOCATION, "The first starting wing and the first team-versus-team wing must have the same wing name.\n");
 	}
 
 	// set up wing indexes
@@ -5610,7 +5633,7 @@ void post_process_ships_wings()
 			for (int i = 1; i < MAX_STARTING_WINGS; i++) {
 				// If there was a wing for this squadron entry, check the last one. If it's empty, we found a mistake, so move the wing names over.
 				if (Squadron_wing_names_found[i] && !Squadron_wing_names_found[i - 1]) {
-					Warning(LOCATION, "Squadron wings are not in the correct order and may cause wings to disappear in multi.\n\nEither wing %s should exist or the %s entry needs to come before it in the list.\n\nPlease go back and fix the mission.", Squadron_wing_names[i - 1], Squadron_wing_names[i]);
+					parse_warning_or_record("Squadron wings are not in the correct order and may cause wings to disappear in multi. Either wing %s should exist or the %s entry needs to come before it in the list — wing names have been swapped.", Squadron_wing_names[i - 1], Squadron_wing_names[i]);
 					char temp_chars[NAME_LENGTH];
 					strcpy_s(temp_chars, Squadron_wing_names[i - 1]);
 					strcpy_s(Squadron_wing_names[i - 1], Squadron_wing_names[i]);
@@ -5648,7 +5671,7 @@ void parse_event(mission *pm)
 		// sanity check on the repeat count variable
 		// _argv[-1] - negative repeat count is now legal; means repeat indefinitely.
 		if ( event->repeat_count == 0 ){
-			Warning(LOCATION, "Repeat count for mission event %s is 0.\nMust be >= 1 or negative!  Setting to 1.", event->name.c_str() );
+			parse_warning_or_record("Repeat count for mission event %s is 0 — must be >= 1 or negative; corrected to 1.", event->name.c_str());
 			event->repeat_count = 1;
 		}
 	}
@@ -5665,7 +5688,7 @@ void parse_event(mission *pm)
 		// sanity check on the trigger count variable
 		// negative trigger count is also legal
 		if ( event->trigger_count == 0 ){
-			Warning(LOCATION, "Trigger count for mission event %s is 0.\nMust be >= 1 or negative!  Setting to 1.", event->name.c_str() );
+			parse_warning_or_record("Trigger count for mission event %s is 0 — must be >= 1 or negative; corrected to 1.", event->name.c_str());
 			event->trigger_count = 1;
 		}
 	}
@@ -6021,7 +6044,7 @@ void parse_reinforcement(mission *pm)
 		stuff_int(&delay);
 		if (delay < 0)
 		{
-			Warning(LOCATION, "Cannot have arrival delay < 0 on reinforcement %s", ptr->name);
+			parse_warning_or_record("Arrival delay on reinforcement %s cannot be negative — corrected to 0.", ptr->name);
 			delay = 0;
 		}
 
@@ -6041,14 +6064,14 @@ void parse_reinforcement(mission *pm)
 
 	if (rforce_obj == NULL) {
 		if ((instance = wing_name_lookup(ptr->name, 1)) == -1) {
-			Warning(LOCATION, "Reinforcement %s not found as ship or wing", ptr->name);
+			parse_warning_or_record("Reinforcement %s not found as ship or wing — declaration ignored.", ptr->name);
 			return;
 		}
 	} else {
 		// Individual ships in wings can't be reinforcements - FUBAR
 		if (rforce_obj->wingnum >= 0)
 		{
-			Warning(LOCATION, "Reinforcement %s is part of a wing - Ignoring reinforcement declaration", ptr->name);
+			parse_warning_or_record("Reinforcement %s is part of a wing — reinforcement declaration ignored.", ptr->name);
 			return;
 		}
 		else
@@ -6698,6 +6721,9 @@ bool parse_mission(mission *pm, int flags)
 	int saved_warning_count = Global_warning_count;
 	int saved_error_count = Global_error_count;
 
+	// Reset the parse-time warning queue so each load starts fresh (only consumed by QtFRED).
+	Mission_parse_warnings.clear();
+
 	// reset parse error stuff
 	Num_unknown_ship_classes = 0;
 	Num_unknown_prop_classes = 0;
@@ -6798,7 +6824,9 @@ bool parse_mission(mission *pm, int flags)
 	if (!post_process_mission(pm))
 		return false;
 
-	if ((saved_warning_count - Global_warning_count) > 10 || (saved_error_count - Global_error_count) > 0) {
+	// QtFRED surfaces parse issues through its own error checker, so skip this summary popup there; Fred2 and the game still show it.
+	if (!Qtfred_running &&
+		((saved_warning_count - Global_warning_count) > 10 || (saved_error_count - Global_error_count) > 0)) {
 		char text[512];
 		sprintf(text, "Warning!\n\nThe current mission has generated %d warnings and/or errors during load.  These are usually caused by corrupted ship models or syntax errors in the mission file.  While FreeSpace Open will attempt to compensate for these issues, it cannot guarantee a trouble-free gameplay experience.  Source Code Project staff cannot provide assistance or support for these problems, as they are caused by the mission's data files, not FreeSpace Open's source code.", (saved_warning_count - Global_warning_count) + (saved_error_count - Global_error_count));
 		popup(PF_TITLE_BIG | PF_TITLE_RED | PF_USE_AFFIRMATIVE_ICON | PF_NO_NETWORKING, 1, POPUP_OK, text);
@@ -6936,7 +6964,9 @@ bool post_process_mission(mission *pm)
 					error_msg += "\n\n(Bad node appears to be: ";
 					error_msg += bad_node_str;
 					error_msg += ")\n";
-					Warning(LOCATION, "%s", error_msg.c_str());
+					// QtFRED surfaces SEXP errors through ErrorChecker's fred_check_sexp; skip the popup there.
+					if (!Qtfred_running)
+						Warning(LOCATION, "%s", error_msg.c_str());
 
 					// syntax errors are recoverable in Fred but not FS
 					if (!Fred_running && !sexp_recoverable_error(result)) {
@@ -7734,15 +7764,17 @@ void mission_parse_set_up_initial_docks()
 		// display an error if necessary
 		if (dfi.maintained_variables.int_value == 0)
 		{
-			Warning(LOCATION, "In the docking group containing %s, every ship has an arrival cue set to false.  The group will not appear in-mission!\n", pobjp->name);
+			if (!Qtfred_running)
+				Warning(LOCATION, "In the docking group containing %s, every ship has an arrival cue set to false.  The group will not appear in-mission!\n", pobjp->name);
 
 			// for FRED, we must arbitrarily choose a dock leader, otherwise the entire docked group will not be loaded
 			if (Fred_running)
 				dock_evaluate_all_docked_objects(pobjp, &dfi, parse_object_choose_arbitrary_dock_leader_helper);
 		}
 		else if (dfi.maintained_variables.int_value > 1)
 		{
-			Warning(LOCATION, "In the docking group containing %s, there is more than one ship with a non-false arrival cue!  There can only be one such ship.  Setting all arrival cues except %s to false...\n", dfi.maintained_variables.objp_value->name, dfi.maintained_variables.objp_value->name);
+			if (!Qtfred_running)
+				Warning(LOCATION, "In the docking group containing %s, there is more than one ship with a non-false arrival cue!  There can only be one such ship.  Setting all arrival cues except %s to false...\n", dfi.maintained_variables.objp_value->name, dfi.maintained_variables.objp_value->name);
 		}
 
 		// clear dfi stuff

code/mission/missionparse.h

@@ -566,6 +566,11 @@ extern fix Mission_end_time;
 
 extern SCP_vector<SCP_string> Parse_names;
 
+// Populated when Qtfred_running and a parse-time auto-correction fires. Drained by
+// QtFRED's ErrorChecker so the corrections are visible to the designer instead of
+// silently buried. Outside of QtFRED these sites still call Warning(LOCATION, ...).
+extern SCP_vector<SCP_string> Mission_parse_warnings;
+
 extern char			Player_start_shipname[NAME_LENGTH];
 extern int			Player_start_shipnum;
 extern p_object	*Player_start_pobject;

fred2/fred.cpp

@@ -106,6 +106,7 @@ pending_message Pending_messages[MAX_PENDING_MESSAGES];
 CFREDApp theApp;
 
 int Fred_running = 1;
+int Qtfred_running = 0;
 int FrameCount = 0;
 bool Fred_active = true;
 int Update_window = 1;

freespace2/freespace.cpp

@@ -399,6 +399,7 @@ int Show_net_stats;
 bool Pre_player_entry = false;
 
 int	Fred_running = 0;
+int	Qtfred_running = 0;
 bool running_unittests = false;
 
 // required for hudtarget... kinda dumb, but meh

qtfred/help-src/doc/dialogs/ErrorCheckerDialog.html

@@ -8,7 +8,82 @@
 <body>
 <nav class="breadcrumb"><a href="../index.html">QtFRED Help</a> &rsaquo; Tools &rsaquo; Error Checker</nav>
 <h1>Error Checker</h1>
-<p>Accessible via <strong>Tools &rsaquo; Error Checker</strong>.</p>
-<div class="stub-notice">Full documentation for this tool is not yet written.</div>
+<p>Opens via <strong>Tools &rsaquo; Error Checker</strong>. Also runs automatically
+before saving when issues are present.</p>
+<p>Scans the entire mission for structural problems, design errors, configuration
+warnings, and potential issues. Results are listed by severity with a description
+of each problem found.</p>
+
+<h2>Controls</h2>
+<table>
+    <tr><th>Control</th><th>Description</th></tr>
+    <tr><td>Rerun</td><td>Runs the check again. Use this after making fixes to confirm
+        they are resolved.</td></tr>
+    <tr><td>Check Potential Issues</td><td>When checked, Potential Issue entries are
+        included in the results alongside errors and warnings. When unchecked,
+        potential issues are silently omitted.</td></tr>
+    <tr><td>Apply Auto-Corrections</td><td>When checked, FRED automatically corrects
+        issues it can resolve without user input each time a check runs. Only applies
+        to correctable errors and warnings; Critical Errors are never auto-corrected.
+        Auto-corrections are applied before results are displayed.</td></tr>
+    <tr><td>Status bar</td><td>Shows a summary of the most recent check: the total
+        number of entries found, or <em>No check has been run yet</em> before the
+        first run.</td></tr>
+</table>
+
+<h2>Severity levels</h2>
+<table>
+    <tr><th>Severity</th><th>Meaning</th></tr>
+    <tr><td>Critical Error</td><td>A serious structural problem that FRED cannot
+        automatically correct. The mission may fail to load or behave unpredictably
+        in-game. Must be fixed manually before the mission is usable.</td></tr>
+    <tr><td>Error</td><td>A mission design problem that must be fixed. The mission may
+        not play correctly.</td></tr>
+    <tr><td>Warning</td><td>A problem that can be (or has been) auto-corrected. Review
+        the change before saving.</td></tr>
+    <tr><td>Potential Issue</td><td>A situation that may be intentional but is worth
+        reviewing. Only shown when <strong>Check Potential Issues</strong> is
+        checked.</td></tr>
+</table>
+
+<h2>What is checked</h2>
+<p>The checker inspects the following areas of the mission:</p>
+<ul>
+    <li>Object list integrity and name uniqueness</li>
+    <li>Ships — SEXPs, AI goals, anchor references, docking configuration, and loadout
+        weapon availability</li>
+    <li>Wings — structure, SEXPs, AI goals, anchor references, wave thresholds, and
+        accepted orders consistency</li>
+    <li>Player starts and player wing membership</li>
+    <li>Waypoint path names for conflicts with object names</li>
+    <li>Reinforcement name references</li>
+    <li>Mission events and mission objectives — SEXP validation</li>
+    <li>Briefings — icon ID uniqueness</li>
+    <li>Debriefings — SEXP validation</li>
+    <li>Asteroid field target ship name validity</li>
+    <li>Initially-docked groups — must have exactly one non-false arrival cue</li>
+    <li>Team loadout — weapons used in starting wings must be present in the
+        team loadout pool</li>
+</ul>
+
+<h2>Pre-save mode</h2>
+<p>When saving a mission that has unresolved errors, the Error Checker opens
+automatically in pre-save mode. In this mode three additional buttons appear at the
+bottom of the dialog:</p>
+<table>
+    <tr><th>Button</th><th>Description</th></tr>
+    <tr><td>Cancel</td><td>Aborts the save and returns to the editor so issues can be
+        addressed.</td></tr>
+    <tr><td>Save As Is</td><td>Saves the mission despite the reported issues.</td></tr>
+    <tr><td>Fix and Save</td><td>Applies all available auto-corrections and then saves.
+        Only available when <strong>Apply Auto-Corrections</strong> is
+        checked.</td></tr>
+</table>
+
+<h2>Preferences</h2>
+<p>The <strong>Check Potential Issues</strong> and <strong>Apply Auto-Corrections</strong>
+settings are also found in <strong>File &rsaquo; Preferences &rsaquo; Error
+Checker</strong>. They share the same stored value — changing one updates the other.
+Use Preferences to set the default behavior for all future check runs.</p>
 </body>
 </html>