From 4cdb674fb81e706f7a8c611fcd6668d24572cc79 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Thu, 26 Feb 2026 13:55:14 +0000 Subject: [PATCH] Correct validation of action probabilities. This fixes a problem first observed in the graphical interface (#788), in which validation of chance probabilities was not being handled correctly. The slightly deeper issue was that validation was not exposed separately. The solution is to refactor and expose a validation function, so the validation logic is available both in setting the probabilities, but also in validating the dialog. This also adds a few small UX and wxWidgets modernisations. Fixes #788. --- src/games/game.h | 15 ++++++++ src/games/gametree.cc | 11 +----- src/gui/dleditmove.cc | 80 +++++++++++++++++-------------------------- src/gui/dleditmove.h | 2 -- 4 files changed, 47 insertions(+), 61 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index f3df5b2dc..8d01bbbe7 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -593,6 +593,21 @@ inline GameNodeRep::Actions::iterator::iterator(GameInfosetRep::Actions::iterato inline GameNode GameNodeRep::Actions::iterator::GetOwner() const { return m_child_it.GetOwner(); } +inline void ValidateDistribution(const Array &p_probs, const bool p_normalized = true) +{ + if (std::any_of(p_probs.begin(), p_probs.end(), + [](const Number &x) { return static_cast(x) < Rational(0); })) { + throw ValueException("Probabilities must be non-negative numbers"); + } + if (!p_normalized) { + return; + } + if (sum_function(p_probs, [](const Number &n) { return static_cast(n); }) != + Rational(1)) { + throw ValueException("Probabilities must sum to exactly one"); + } +} + enum class TraversalOrder { Preorder, Postorder }; class CartesianProductSpace { diff --git a/src/games/gametree.cc b/src/games/gametree.cc index e3e6b656e..8933e1bb0 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -1324,17 +1324,8 @@ Game GameTreeRep::SetChanceProbs(const GameInfoset &p_infoset, const Arraym_actions.size() != p_probs.size()) { throw DimensionException("The number of probabilities given must match the number of actions"); } + ValidateDistribution(p_probs); IncrementVersion(); - if (std::any_of(p_probs.begin(), p_probs.end(), - [](const Number &x) { return static_cast(x) < Rational(0); })) { - throw ValueException("Probabilities must be non-negative numbers"); - } - auto sum = std::accumulate( - p_probs.begin(), p_probs.end(), Rational(0), - [](const Rational &r, const Number &n) { return r + static_cast(n); }); - if (sum != Rational(1)) { - throw ValueException("Probabilities must sum to exactly one"); - } std::copy(p_probs.begin(), p_probs.end(), p_infoset->m_probs.begin()); ClearComputedValues(); return shared_from_this(); diff --git a/src/gui/dleditmove.cc b/src/gui/dleditmove.cc index 0706f31e1..d52d9b44d 100644 --- a/src/gui/dleditmove.cc +++ b/src/gui/dleditmove.cc @@ -32,8 +32,11 @@ #include "renratio.h" namespace Gambit::GUI { + class ActionSheet final : public wxSheet { - GameInfoset m_infoset; + GameInfoset m_infoset{nullptr}; + wxSheetCellAttr m_labelAttr; + wxFont m_labelFont, m_cellFont; // Overriding wxSheet members wxSheetCellAttr GetAttr(const wxSheetCoords &p_coords, wxSheetAttr_Type) const override; @@ -48,8 +51,17 @@ class ActionSheet final : public wxSheet { }; ActionSheet::ActionSheet(wxWindow *p_parent, const GameInfoset &p_infoset) - : wxSheet(p_parent, wxID_ANY), m_infoset(p_infoset) + : wxSheet(p_parent, wxID_ANY), m_infoset(p_infoset), m_labelFont(GetFont()), + m_cellFont(GetFont()) { + m_labelFont.MakeBold(); + + m_labelAttr = GetSheetRefData()->m_defaultRowLabelAttr; + m_labelAttr.SetFont(m_labelFont); + m_labelAttr.SetAlignment(wxALIGN_CENTER, wxALIGN_CENTER); + m_labelAttr.SetOrientation(wxHORIZONTAL); + m_labelAttr.SetReadOnly(true); + CreateGrid(p_infoset->GetActions().size(), (p_infoset->IsChanceInfoset()) ? 2 : 1); SetRowLabelWidth(40); SetColLabelHeight(25); @@ -103,29 +115,12 @@ Array ActionSheet::GetActionProbs() wxSheetCellAttr ActionSheet::GetAttr(const wxSheetCoords &p_coords, wxSheetAttr_Type) const { - if (IsRowLabelCell(p_coords)) { - wxSheetCellAttr attr(GetSheetRefData()->m_defaultRowLabelAttr); - attr.SetFont(wxFont(10, wxFONTFAMILY_SWISS, wxFONTSTYLE_NORMAL, wxFONTWEIGHT_BOLD)); - attr.SetAlignment(wxALIGN_CENTER, wxALIGN_CENTER); - attr.SetOrientation(wxHORIZONTAL); - attr.SetReadOnly(true); - return attr; - } - else if (IsColLabelCell(p_coords)) { - wxSheetCellAttr attr(GetSheetRefData()->m_defaultColLabelAttr); - attr.SetFont(wxFont(10, wxFONTFAMILY_SWISS, wxFONTSTYLE_NORMAL, wxFONTWEIGHT_BOLD)); - attr.SetAlignment(wxALIGN_CENTER, wxALIGN_CENTER); - attr.SetOrientation(wxHORIZONTAL); - attr.SetReadOnly(true); - return attr; - } - else if (IsCornerLabelCell(p_coords)) { - const wxSheetCellAttr attr(GetSheetRefData()->m_defaultCornerLabelAttr); - return attr; + if (IsLabelCell(p_coords)) { + return m_labelAttr; } wxSheetCellAttr attr(GetSheetRefData()->m_defaultGridCellAttr); - attr.SetFont(wxFont(10, wxFONTFAMILY_SWISS, wxFONTSTYLE_NORMAL, wxFONTWEIGHT_NORMAL)); + attr.SetFont(m_cellFont); attr.SetAlignment(wxALIGN_CENTER, wxALIGN_CENTER); attr.SetOrientation(wxHORIZONTAL); attr.SetReadOnly(false); @@ -143,9 +138,7 @@ wxSheetCellAttr ActionSheet::GetAttr(const wxSheetCoords &p_coords, wxSheetAttr_ // class EditMoveDialog //====================================================================== -wxBEGIN_EVENT_TABLE(EditMoveDialog, wxDialog) EVT_BUTTON(wxID_OK, EditMoveDialog::OnOK) - wxEND_EVENT_TABLE() EditMoveDialog::EditMoveDialog(wxWindow *p_parent, - const GameInfoset &p_infoset) +EditMoveDialog::EditMoveDialog(wxWindow *p_parent, const GameInfoset &p_infoset) : wxDialog(p_parent, wxID_ANY, _("Move properties"), wxDefaultPosition), m_infoset(p_infoset) { auto *topSizer = new wxBoxSizer(wxVERTICAL); @@ -171,6 +164,7 @@ wxBEGIN_EVENT_TABLE(EditMoveDialog, wxDialog) EVT_BUTTON(wxID_OK, EditMoveDialog if (p_infoset->IsChanceInfoset()) { m_player->Append(_("Chance")); m_player->SetSelection(0); + m_player->Disable(); } else { for (const auto &player : p_infoset->GetGame()->GetPlayers()) { @@ -189,18 +183,13 @@ wxBEGIN_EVENT_TABLE(EditMoveDialog, wxDialog) EVT_BUTTON(wxID_OK, EditMoveDialog actionBoxSizer->Add(m_actionSheet, 1, wxALL | wxEXPAND, 5); topSizer->Add(actionBoxSizer, 0, wxALL | wxEXPAND, 5); - auto *buttonSizer = new wxBoxSizer(wxHORIZONTAL); - buttonSizer->Add(new wxButton(this, wxID_CANCEL, _("Cancel")), 0, wxALL, 5); - auto *okButton = new wxButton(this, wxID_OK, _("OK")); - okButton->SetDefault(); - buttonSizer->Add(okButton, 0, wxALL, 5); - topSizer->Add(buttonSizer, 0, wxALL | wxALIGN_RIGHT, 5); - - SetSizer(topSizer); - topSizer->Fit(this); - topSizer->SetSizeHints(this); - wxTopLevelWindowBase::Layout(); + if (auto *buttons = CreateSeparatedButtonSizer(wxOK | wxCANCEL)) { + topSizer->Add(buttons, 0, wxALL | wxEXPAND, 5); + } + SetSizerAndFit(topSizer); CenterOnParent(); + + Bind(wxEVT_BUTTON, &EditMoveDialog::OnOK, this, wxID_OK); } void EditMoveDialog::OnOK(wxCommandEvent &p_event) @@ -209,23 +198,16 @@ void EditMoveDialog::OnOK(wxCommandEvent &p_event) p_event.Skip(); return; } - auto probs = m_actionSheet->GetActionProbs(); - if (std::any_of(probs.begin(), probs.end(), - [](const Number &p) { return static_cast(p) < Rational(0); })) { - wxMessageBox("Action probabilities must be non-negative numbers.", "Error"); - return; - } - if (std::accumulate(probs.begin(), probs.end(), Rational(0), - [](const Rational &s, const Number &p) { - return s + static_cast(p); - }) != Rational(1)) { - p_event.Skip(); + try { + ValidateDistribution(m_actionSheet->GetActionProbs()); } - else { - wxRichMessageDialog(this, "Action probabilities must sum to exactly one", "Error", + catch (ValueException &) { + wxRichMessageDialog(this, "Probabilities must be nonnegative numbers summing to one.", "Error", wxOK | wxCENTRE | wxICON_ERROR) .ShowModal(); + return; } + p_event.Skip(); } int EditMoveDialog::NumActions() const { return m_actionSheet->NumActions(); } diff --git a/src/gui/dleditmove.h b/src/gui/dleditmove.h index 45355c773..fa0594e3f 100644 --- a/src/gui/dleditmove.h +++ b/src/gui/dleditmove.h @@ -45,8 +45,6 @@ class EditMoveDialog final : public wxDialog { int NumActions() const; wxString GetActionName(int p_act) const; Array GetActionProbs() const; - - wxDECLARE_EVENT_TABLE(); }; } // namespace Gambit::GUI