Merge pull request scp-fs2open#7591 from Goober5000/fix/icon_propagation

fix several bugs related to icon propagation, mostly in FRED

Author: Goober5000

fred2/briefingeditordlg.cpp

@@ -432,7 +432,18 @@ void briefing_editor_dlg::update_data(int update)
 
 		if (m_last_icon >= 0) {
 			valid = (m_id != ptr->icons[m_last_icon].id);
-			if (m_id >= 0) {
+			if (m_id < 0) {
+				// briefing icon ids must never be negative; revert to the previous id
+				if (valid) {
+					char msg[1024];
+
+					sprintf(msg, "Icon ID #%d is invalid.  Briefing icon IDs cannot be negative.\n"
+						"Icon ID has been reset back to %d", m_id, ptr->icons[m_last_icon].id);
+
+					m_id = ptr->icons[m_last_icon].id;
+					MessageBox(msg);
+				}
+			} else {
 				if (valid && !m_change_local) {
 					for (i=m_last_stage+1; i<Briefing->num_stages; i++) {
 						if (find_icon(m_id, i) >= 0) {
@@ -473,7 +484,7 @@ void briefing_editor_dlg::update_data(int update)
 
 			string_copy(buf, m_icon_label, MAX_LABEL_LEN - 1);
 			lcl_fred_replace_stuff(buf, MAX_LABEL_LEN - 1);
-			if (stricmp(ptr->icons[m_last_icon].label, buf) && !m_change_local) {
+			if (strcmp(ptr->icons[m_last_icon].label, buf) && !m_change_local) {
 				set_modified();
 				reset_icon_loop(m_last_stage);
 				while (get_next_icon(m_id))
@@ -483,36 +494,41 @@ void briefing_editor_dlg::update_data(int update)
 
 			string_copy(buf, m_icon_closeup_label, MAX_LABEL_LEN - 1);
 			lcl_fred_replace_stuff(buf, MAX_LABEL_LEN - 1);
-			if (stricmp(ptr->icons[m_last_icon].closeup_label, buf) && !m_change_local) {
+			if (strcmp(ptr->icons[m_last_icon].closeup_label, buf) && !m_change_local) {
 				set_modified();
 				reset_icon_loop(m_last_stage);
 				while (get_next_icon(m_id))
 					strcpy_s(iconp->closeup_label, buf);
 			}
 			strcpy_s(ptr->icons[m_last_icon].closeup_label, buf);
 
-			if (m_icon_scale > 0)
-				ptr->icons[m_last_icon].scale_factor = m_icon_scale / 100.0f;
-
-			if ( m_hilight )
-				ptr->icons[m_last_icon].flags |= BI_HIGHLIGHT;
-			else
-				ptr->icons[m_last_icon].flags &= ~BI_HIGHLIGHT;
-
-			if (m_flipicon)
-				ptr->icons[m_last_icon].flags |= BI_MIRROR_ICON;
-			else
-				ptr->icons[m_last_icon].flags &= ~BI_MIRROR_ICON;
+			if (m_icon_scale > 0) {
+				float new_scale = m_icon_scale / 100.0f;
+				if ((ptr->icons[m_last_icon].scale_factor != new_scale) && !m_change_local) {
+					set_modified();
+					reset_icon_loop(m_last_stage);
+					while (get_next_icon(m_id))
+						iconp->scale_factor = new_scale;
+				}
+				ptr->icons[m_last_icon].scale_factor = new_scale;
+			}
 
-			if (m_use_wing)
-				ptr->icons[m_last_icon].flags |= BI_USE_WING_ICON;
-			else
-				ptr->icons[m_last_icon].flags &= ~BI_USE_WING_ICON;
+			// build the new flags for this icon from the checkboxes, preserving any non-editable bits
+			const int editable_flags = BI_HIGHLIGHT | BI_MIRROR_ICON | BI_USE_WING_ICON | BI_USE_CARGO_ICON;
+			int new_flags = ptr->icons[m_last_icon].flags;
+			if (m_hilight)   new_flags |= BI_HIGHLIGHT;      else new_flags &= ~BI_HIGHLIGHT;
+			if (m_flipicon)  new_flags |= BI_MIRROR_ICON;    else new_flags &= ~BI_MIRROR_ICON;
+			if (m_use_wing)  new_flags |= BI_USE_WING_ICON;  else new_flags &= ~BI_USE_WING_ICON;
+			if (m_use_cargo) new_flags |= BI_USE_CARGO_ICON; else new_flags &= ~BI_USE_CARGO_ICON;
 
-			if (m_use_cargo)
-				ptr->icons[m_last_icon].flags |= BI_USE_CARGO_ICON;
-			else
-				ptr->icons[m_last_icon].flags &= ~BI_USE_CARGO_ICON;
+			if ((ptr->icons[m_last_icon].flags != new_flags) && !m_change_local) {
+				set_modified();
+				reset_icon_loop(m_last_stage);
+				// apply only the editable bits to later icons, preserving their other flags
+				while (get_next_icon(m_id))
+					iconp->flags = (iconp->flags & ~editable_flags) | (new_flags & editable_flags);
+			}
+			ptr->icons[m_last_icon].flags = new_flags;
 
 			if ((ptr->icons[m_last_icon].type != m_icon_image) && !m_change_local) {
 				set_modified();
@@ -942,6 +958,7 @@ void briefing_editor_dlg::copy_stage(int from, int to)
 		Briefing->stages[to].camera_time = 500;
 		Briefing->stages[to].num_icons = 0;
 		Briefing->stages[to].formula = Locked_sexp_true;
+		Briefing->stages[to].draw_grid = true;
 		Briefing->stages[to].grid_color = Color_briefing_grid;
 		return;
 	}
@@ -956,6 +973,7 @@ void briefing_editor_dlg::copy_stage(int from, int to)
 	Briefing->stages[to].num_icons = Briefing->stages[from].num_icons;
 	Briefing->stages[to].num_lines = Briefing->stages[from].num_lines;
 	Briefing->stages[to].formula = Briefing->stages[from].formula;
+	Briefing->stages[to].draw_grid = Briefing->stages[from].draw_grid;
 	// For now let's just always set this back to default. Eventually when we have a UI color picker in qtFRED, we can copy from stage to stage
 	Briefing->stages[to].grid_color = Color_briefing_grid;
 
@@ -1089,12 +1107,13 @@ void briefing_editor_dlg::OnMakeIcon()
 
 	strncpy(biconp->label, name, len);
 	biconp->label[len] = 0;
+	biconp->closeup_label[0] = 0;
 //	iconp->text[0] = 0;
 	biconp->type = 0;
 	biconp->team = team;
 	biconp->pos = pos;
 	biconp->flags = 0;
-	biconp->id = Cur_brief_id++;
+	biconp->id = get_unused_briefing_icon_id();
 	biconp->scale_factor = 1.0f;
 
 	biconp->modelnum = -1;
@@ -1300,10 +1319,13 @@ int briefing_editor_dlg::check_mouse_hit(int x, int y)
 	return -1;
 }
 
-void briefing_editor_dlg::OnPropagateIcons() 
+void briefing_editor_dlg::OnPropagateIcons()
 {
 	object *ptr;
 
+	// commit any pending edits in the icon fields before propagating the icons forward
+	update_data(1);
+
 	ptr = GET_FIRST(&obj_used_list);
 	while (ptr != END_OF_LIST(&obj_used_list)) {
 		if ((ptr->type == OBJ_POINT) && (ptr->flags[Object::Object_Flags::Marked])) {
@@ -1343,6 +1365,26 @@ int briefing_editor_dlg::find_icon(int id, int stage)
 	return -1;
 }
 
+// is the given id already used by any icon in any stage of the current briefing?
+bool briefing_editor_dlg::briefing_icon_id_used(int id)
+{
+	for (int s=0; s<Briefing->num_stages; s++)
+		if (find_icon(id, s) >= 0)
+			return true;
+
+	return false;
+}
+
+// find an id that isn't already used by an icon in the current briefing, advancing
+// Cur_brief_id past any values that are already taken (e.g. from manual id edits)
+int briefing_editor_dlg::get_unused_briefing_icon_id()
+{
+	while (briefing_icon_id_used(Cur_brief_id))
+		Cur_brief_id++;
+
+	return Cur_brief_id++;
+}
+
 void briefing_editor_dlg::reset_icon_loop(int stage)
 {
 	stage_loop = stage + 1;
@@ -1355,7 +1397,7 @@ int briefing_editor_dlg::get_next_icon(int id)
 		icon_loop++;
 		if (icon_loop >= Briefing->stages[stage_loop].num_icons) {
 			stage_loop++;
-			if (stage_loop > Briefing->num_stages)
+			if (stage_loop >= Briefing->num_stages)
 				return 0;
 
 			icon_loop = -1;

fred2/briefingeditordlg.h

@@ -34,6 +34,8 @@ class briefing_editor_dlg : public CDialog
 	void OnOK();
 	void OnCancel();
 	int find_icon(int id, int stage);
+	bool briefing_icon_id_used(int id);
+	int get_unused_briefing_icon_id();
 	void propagate_icon(int num);
 	void reset_editor();
 	int check_mouse_hit(int x, int y);

qtfred/src/mission/dialogs/BriefingEditorDialogModel.cpp

@@ -785,9 +785,68 @@ int BriefingEditorDialogModel::getIconId() const
 	return s.icons[_currentIcon].id;
 }
 
-void BriefingEditorDialogModel::setIconId(int id)
+bool BriefingEditorDialogModel::setIconId(int id)
 {
-	applyToSelectedIconsCurrentAndForward([&](brief_icon& ic) { modify(ic.id, id); });
+	auto& briefing = _wipBriefings[_currentTeam];
+	if (briefing.num_stages <= 0 || _currentStage < 0 || _currentStage >= briefing.num_stages)
+		return true;
+
+	auto& stage = briefing.stages[_currentStage];
+	if (_currentIcon < 0 || _currentIcon >= stage.num_icons)
+		return true;
+
+	// an id applies to a single icon, so operate on the current icon rather than the
+	// whole selection; applying one id to several icons would itself create a collision
+	const int oldId = stage.icons[_currentIcon].id;
+	if (id == oldId)
+		return true; // no change
+
+	// briefing icon ids must never be negative (the spin box also enforces this)
+	if (id < 0)
+		return false;
+
+	// an icon id must be unique within its stage
+	for (int i = 0; i < stage.num_icons; ++i) {
+		if (i != _currentIcon && stage.icons[i].id == id) {
+			QMessageBox::warning(nullptr,
+				tr("Icon ID"),
+				tr("Icon ID %1 is already used by another icon in this stage.  The ID has not been changed.").arg(id));
+			return false;
+		}
+	}
+
+	// when propagating forward, the new id must not already be used by a different icon in a
+	// later stage, or the global rename would merge two distinct icon chains into one
+	if (!_changeLocally) {
+		for (int st = _currentStage + 1; st < briefing.num_stages; ++st) {
+			const auto& s = briefing.stages[st];
+			for (int i = 0; i < s.num_icons; ++i) {
+				if (s.icons[i].id == id) {
+					QMessageBox::warning(nullptr,
+						tr("Icon ID"),
+						tr("Icon ID %1 is already used in a later stage.  You can only change to that ID "
+						   "locally.  The ID has not been changed.").arg(id));
+					return false;
+				}
+			}
+		}
+	}
+
+	// apply the change to the current icon, and (unless local-only) to the same icon in later stages
+	stage.icons[_currentIcon].id = id;
+	if (!_changeLocally) {
+		for (int st = _currentStage + 1; st < briefing.num_stages; ++st) {
+			auto& s = briefing.stages[st];
+			for (int i = 0; i < s.num_icons; ++i) {
+				if (s.icons[i].id == oldId)
+					s.icons[i].id = id;
+			}
+		}
+	}
+
+	set_modified();
+	modelChanged();
+	return true;
 }
 
 SCP_string BriefingEditorDialogModel::getIconLabel() const

qtfred/src/mission/dialogs/BriefingEditorDialogModel.h

@@ -74,7 +74,8 @@ class BriefingEditorDialogModel : public AbstractDialogModel {
 	vec3d getIconPosition() const;
 	void setIconPosition(const vec3d& pos);
 	int getIconId() const;
-	void setIconId(int id);
+	// returns false if the requested id was rejected (e.g. it collides with another icon)
+	bool setIconId(int id);
 	SCP_string getIconLabel() const;
 	void setIconLabel(const SCP_string& text);
 	SCP_string getIconCloseupLabel() const;

qtfred/src/ui/dialogs/BriefingEditorDialog.cpp

@@ -89,6 +89,10 @@ BriefingEditorDialog::BriefingEditorDialog(FredView* parent, EditorViewport* vie
 	ui->iconCloseupLabelLineEdit->setMaxLength(MAX_LABEL_LEN - 1);
 	ui->voiceFileLineEdit->setMaxLength(MAX_FILENAME_LEN - 1);
 
+	// validate the icon id when the edit is committed (focus-out/Enter) rather than on every
+	// keystroke, so a transient value while typing can't pop a collision warning
+	ui->iconIdSpinBox->setKeyboardTracking(false);
+
 	setupMapWidget();
 	initializeUi();
 	updateUi();
@@ -562,7 +566,12 @@ void BriefingEditorDialog::on_disableGridCheckBox_toggled(bool checked)
 
 void BriefingEditorDialog::on_iconIdSpinBox_valueChanged(int arg1)
 {
-	_model->setIconId(arg1);
+	if (!_model->setIconId(arg1)) {
+		// the id was rejected; reset the spin box to the model's current value
+		ui->iconIdSpinBox->blockSignals(true);
+		ui->iconIdSpinBox->setValue(_model->getIconId());
+		ui->iconIdSpinBox->blockSignals(false);
+	}
 }
 
 void BriefingEditorDialog::on_iconLabelLineEdit_textChanged(const QString& string)