Condition Evaluation by DarkiBoi · Pull Request #698 · OpenVicProject/OpenVic-Simulation

DarkiBoi · 2026-02-15T15:05:41Z

No description provided.

Copilot

Pull request overview

Introduces an initial runtime evaluation path for scripted conditions by adding an evaluation Context and wiring ConditionNode evaluation through scope redirection/iteration, while also extending condition registration metadata (localisation keys + tooltip subject) via a builder API.

Changes:

Add Context (variant of scope pointers + THIS/FROM tracking) to support scope changes and iterators during condition evaluation.
Add ConditionNode::evaluate() / evaluate_group() and delegate leaf evaluation to scope objects (CountryInstance/ProvinceInstance/State/Pop).
Extend Condition registration with localisation/tooltip metadata and replace direct registration calls with a ConditionBuilder fluent API.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
src/openvic-simulation/scripts/Context.hpp	Defines evaluation context, including scope pointer variant and THIS/FROM linkage.
src/openvic-simulation/scripts/Context.cpp	Implements scope identification, identifier access, leaf dispatch, iterator sub-contexts, and redirect contexts.
src/openvic-simulation/scripts/Condition.hpp	Adds tooltip metadata fields and declares `ConditionNode` evaluation entrypoints + builder-based condition registration.
src/openvic-simulation/scripts/Condition.cpp	Implements condition evaluation logic and migrates condition registration to builder API.
src/openvic-simulation/population/Pop.hpp / Pop.cpp	Adds pop-scope leaf evaluation (partial) for conditions.
src/openvic-simulation/map/State.hpp / State.cpp	Adds state-scope leaf evaluation stub and wires includes.
src/openvic-simulation/map/ProvinceInstance.hpp / ProvinceInstance.cpp	Adds province-scope leaf evaluation stub and wires includes.
src/openvic-simulation/country/CountryInstance.hpp / CountryInstance.cpp	Adds country-scope leaf evaluation (partial) and wires includes.

Comments suppressed due to low confidence (1)

src/openvic-simulation/scripts/Condition.cpp:211

AND/OR/NOT are registered with scope COUNTRY, but ConditionNode::evaluate() enforces share_scope_type(condition->scope, context.get_scope_type()). Since AND is also the root condition, this makes any condition script evaluated in a non-country context (province/state/pop) immediately fail at the root. Register these logical operators with a scope mask that includes all valid scopes (e.g., POP|PROVINCE|STATE|COUNTRY) or exempt them from scope checks.

	/* Special Conditions */
	ret &= add_condition("AND", GROUP, COUNTRY);
	ret &= add_condition("OR", GROUP, COUNTRY);
	ret &= add_condition("NOT", GROUP, COUNTRY);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/openvic-simulation/scripts/Context.cpp

src/openvic-simulation/scripts/Condition.cpp

src/openvic-simulation/population/Pop.cpp

src/openvic-simulation/country/CountryInstance.cpp

src/openvic-simulation/scripts/Condition.cpp

src/openvic-simulation/scripts/Context.cpp

src/openvic-simulation/scripts/Condition.cpp

src/openvic-simulation/map/State.cpp

src/openvic-simulation/country/CountryInstance.cpp

Copilot · 2026-02-15T15:41:13Z

src/openvic-simulation/scripts/Context.cpp

+			return p->get_identifier();
+		}
+		else if constexpr (std::is_same_v<T, State const*>) {
+			return p->get_identifier();


Context::get_identifier() returns std::string_view for State by forwarding State::get_identifier(), but State::get_identifier() returns a memory::string by value. This will produce a dangling string_view (view into a temporary) and can lead to use-after-free. Consider returning a stable view (e.g., change State::get_identifier() to return std::string_view to stored data) or change Context::get_identifier() to return an owning string type when the scope is State.

Suggested change

return p->get_identifier();

// State::get_identifier() returns a string by value; storing it in a

// thread_local buffer ensures the returned string_view does not dangle.

thread_local std::string state_identifier_buffer;

state_identifier_buffer = p->get_identifier();

return std::string_view{ state_identifier_buffer };

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 15 comments.

Comments suppressed due to low confidence (1)

src/openvic-simulation/scripts/Condition.cpp:699

After selecting the appropriate overload and storing it in active_condition, the code continues to use properties from the original condition object (lines 694-699) instead of from active_condition. This means the overload selection has no effect during parsing, causing incorrect behavior when an overload should be used. All references to condition after this point should use active_condition instead.

		const std::string_view identifier = condition.get_identifier();
		const value_type_t value_type = condition.value_type;
		const scope_type_t scope = condition.scope;
		const scope_type_t scope_change = condition.scope_change;
		const identifier_type_t key_identifier_type = condition.key_identifier_type;
		const identifier_type_t value_identifier_type = condition.value_identifier_type;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-18T19:56:18Z

src/openvic-simulation/country/CountryInstance.cpp

+		return get_war_exhaustion() >= expected;
+	}
+
+	spdlog::warn_s("Condition {} not implemented in CountryInstance::evaluate_leaf", node.get_condition() ? node.get_condition()->get_identifier() : "NULL");


The error message duplicates the condition null-check logic unnecessarily. Since id is already obtained from node.get_condition()->get_identifier() at line 1183, and would have crashed earlier if condition were null, the ternary operator here is redundant. The message should simply use id for consistency.

Suggested change

spdlog::warn_s("Condition {} not implemented in CountryInstance::evaluate_leaf", node.get_condition() ? node.get_condition()->get_identifier() : "NULL");

spdlog::warn_s("Condition {} not implemented in CountryInstance::evaluate_leaf", id);

Copilot · 2026-02-18T19:56:18Z

src/openvic-simulation/scripts/Condition.cpp

-			effective_current_scope = from_scope;
-		}
-
 		if (!share_scope_type(scope, effective_current_scope) && effective_current_scope > scope) {


After selecting the appropriate overload into active_condition (lines 683-692), this scope validation continues to use scope from the original condition instead of active_condition->scope. This causes incorrect scope validation when an overload is selected, potentially rejecting valid conditions or accepting invalid ones.

Copilot · 2026-02-18T19:56:18Z

src/openvic-simulation/scripts/Condition.hpp

+		const tooltip_subject_t tooltip_subject;
+		const tooltip_position_t tooltip_position;
+
+		mutable std::vector<Condition> overloads;


The overloads vector is declared as mutable, which allows it to be modified even when the Condition object is const. This can lead to thread-safety issues if multiple threads try to register overloads simultaneously for the same condition. Consider using proper synchronization or redesigning the overload storage to be non-mutable and populated during a single-threaded initialization phase.

Suggested change

mutable std::vector<Condition> overloads;

std::vector<Condition> overloads;

Copilot · 2026-02-18T19:56:19Z

src/openvic-simulation/scripts/Context.hpp

+		Context(CountryInstance const* p) : ptr(p), this_scope(this) {}
+		Context(ProvinceInstance const* p) : ptr(p), this_scope(this) {}
+		Context(State const* p) : ptr(p), this_scope(this) {}
+		Context(Pop const* p) : ptr(p), this_scope(this) {}


The simple constructors (lines 25-28) initialize this_scope to this, creating a self-reference. This means that when a Context is used, this_scope will point to the same Context object. However, if this Context object is temporary or moved, the pointer becomes dangling. Consider documenting this lifetime requirement or ensuring that Context objects are always stored in stable locations when used.

Copilot · 2026-02-18T19:56:19Z

src/openvic-simulation/scripts/Condition.cpp

-	ret &= add_condition("can_build_factory_in_capital_state", IDENTIFIER, COUNTRY, NO_SCOPE, NO_IDENTIFIER, BUILDING);
+	ret &= add_condition("administration_spending", REAL, COUNTRY).loc("CRIME_FIGHTING_SPENDING_OVER");
+	ret &= add_condition("ai", BOOLEAN, COUNTRY).loc("IS_AI", "IS_NOT_AI");
+	ret &= add_condition("alliance_with", IDENTIFIER, COUNTRY).value_identifier_type(COUNTRY_TAG).loc("HAVE_ALLIANCE_WITH", "HAVE_N0T_ALLIANCE_WITH").subject(tooltip_subject_t::VALUE);


The localization key "HAVE_N0T_ALLIANCE_WITH" contains a typo: the letter 'O' is replaced with the digit '0' (zero). It should be "HAVE_NOT_ALLIANCE_WITH".

Suggested change

ret &= add_condition("alliance_with", IDENTIFIER, COUNTRY).value_identifier_type(COUNTRY_TAG).loc("HAVE_ALLIANCE_WITH", "HAVE_N0T_ALLIANCE_WITH").subject(tooltip_subject_t::VALUE);

ret &= add_condition("alliance_with", IDENTIFIER, COUNTRY).value_identifier_type(COUNTRY_TAG).loc("HAVE_ALLIANCE_WITH", "HAVE_NOT_ALLIANCE_WITH").subject(tooltip_subject_t::VALUE);

Copilot · 2026-02-18T19:56:20Z

src/openvic-simulation/scripts/Condition.cpp

+	if (id == "NOT") {
+		for (auto const& node : children) {
+			if (node.evaluate(context)) return false;
+		}
+		return true;
+	}


NOT with an empty children list returns true. This is mathematically correct (the negation of "all are false" when there are none), but consider whether an empty NOT should be treated as invalid to prevent confusion.

Copilot · 2026-02-18T19:56:20Z

src/openvic-simulation/country/CountryInstance.cpp

+		fixed_point_t expected = std::get<fixed_point_t>(node.get_value());
+		return get_industrial_power_untracked() >= expected;


The "industrial_score" condition can accept either REAL or IDENTIFIER values according to its definition (line 299), but this implementation only handles the REAL case. When used with an IDENTIFIER (e.g., "industrial_score = ENG" to compare against another country's score), this code will throw a std::bad_variant_access exception when trying to std::get<fixed_point_t>. The implementation should check which variant type is present and handle both cases appropriately.

Suggested change

fixed_point_t expected = std::get<fixed_point_t>(node.get_value());

return get_industrial_power_untracked() >= expected;

return std::visit(

[this](auto const& value) -> bool {

using T = std::decay_t<decltype(value)>;

if constexpr (std::is_same_v<T, fixed_point_t>) {

return get_industrial_power_untracked() >= value;

} else {

Logger::error(

"industrial_score condition received unsupported value type");

return false;

}

},

node.get_value()

);

Copilot · 2026-02-18T19:56:21Z

src/openvic-simulation/map/ProvinceInstance.cpp

 }

+bool ProvinceInstance::evaluate_leaf(ConditionNode const& node) const {
+	// TODO: implement


ProvinceInstance::evaluate_leaf is implemented as a stub that always returns false without logging a warning. This is inconsistent with other evaluate_leaf implementations (Pop, State, CountryInstance) which log warnings for unimplemented conditions. Consider adding a warning log similar to the other implementations to help with debugging.

Suggested change

// TODO: implement

spdlog::warn_s("ProvinceInstance::evaluate_leaf is not yet implemented; returning false for condition.");

Copilot · 2026-02-18T19:56:21Z

src/openvic-simulation/scripts/Condition.cpp

+		return false;
+	}
+
+	if (!share_scope_type(condition->scope, context.get_scope_type())) {


If condition is nullptr (from the default constructor parameter), this will cause a null pointer dereference. While the constructor marks the node as invalid when condition is null, the evaluate function should add a null check before dereferencing condition->scope to prevent crashes.

Copilot · 2026-02-18T19:56:21Z

src/openvic-simulation/scripts/Condition.cpp

+		for (auto const& sub : sub_contexts) {
+			bool all_match = true;
+			for (auto const& node : children) {
+				if (!node.evaluate(sub)) {
+					all_match = false;
+					break;
+				}
+			}
+			if (all_match) {
+				return true;
+			}
+		}
+		return false;


The logic for "any_" iterators appears to require ALL children to match on at least one sub-context, but the expected behavior for "any_owned_province { condition }" should be "there exists at least one owned province where the condition is true", not "there exists a province where all conditions are true". The current implementation at lines 99-111 is correct, but consider adding a clarifying comment about this non-obvious all-match-per-element behavior for "any_" scope changes.

wvpm

The condition ids are not related to the types (province, pop, country, etc).
Please move them outside. The types are already massive and don't need tight coupling with conditions.
A condition can simply receive an instance and perform its checks from the outside.
All data required for conditions is exposed via public const fields or methods.

wvpm · 2026-02-19T14:20:11Z

src/openvic-simulation/country/CountryInstance.cpp

+
+	if (id == "badboy") {
+		fixed_point_t expected_ratio = std::get<fixed_point_t>(node.get_value());
+		return get_infamy_untracked() >= (expected_ratio * fixed_point_t(25));


This is not a hardcoded value.
The bad boy limit comes from the country defines.

DarkiBoi requested a review from Copilot February 15, 2026 15:33

Copilot started reviewing on behalf of DarkiBoi February 15, 2026 15:34 View session

Copilot AI reviewed Feb 15, 2026

View reviewed changes

condition evaluation wip

eb5ef0e

DarkiBoi force-pushed the conditions branch from aa04875 to eb5ef0e Compare February 18, 2026 19:49

DarkiBoi requested a review from Copilot February 18, 2026 19:50

Copilot started reviewing on behalf of DarkiBoi February 18, 2026 19:50 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

wvpm requested changes Feb 19, 2026

View reviewed changes

wvpm reviewed Feb 19, 2026

View reviewed changes

-			return p->get_identifier();
+			// State::get_identifier() returns a string by value; storing it in a
+			// thread_local buffer ensures the returned string_view does not dangle.
+			thread_local std::string state_identifier_buffer;
+			state_identifier_buffer = p->get_identifier();
+			return std::string_view{ state_identifier_buffer };

	spdlog::warn_s("Condition {} not implemented in CountryInstance::evaluate_leaf", node.get_condition() ? node.get_condition()->get_identifier() : "NULL");
	spdlog::warn_s("Condition {} not implemented in CountryInstance::evaluate_leaf", id);

	mutable std::vector<Condition> overloads;
	std::vector<Condition> overloads;

	ret &= add_condition("alliance_with", IDENTIFIER, COUNTRY).value_identifier_type(COUNTRY_TAG).loc("HAVE_ALLIANCE_WITH", "HAVE_N0T_ALLIANCE_WITH").subject(tooltip_subject_t::VALUE);
	ret &= add_condition("alliance_with", IDENTIFIER, COUNTRY).value_identifier_type(COUNTRY_TAG).loc("HAVE_ALLIANCE_WITH", "HAVE_NOT_ALLIANCE_WITH").subject(tooltip_subject_t::VALUE);

		fixed_point_t expected = std::get<fixed_point_t>(node.get_value());
		return get_industrial_power_untracked() >= expected;

-		fixed_point_t expected = std::get<fixed_point_t>(node.get_value());
-		return get_industrial_power_untracked() >= expected;
+		return std::visit(
+			[this](auto const& value) -> bool {
+				using T = std::decay_t<decltype(value)>;
+				if constexpr (std::is_same_v<T, fixed_point_t>) {
+					return get_industrial_power_untracked() >= value;
+				} else {
+					Logger::error(
+						"industrial_score condition received unsupported value type");
+					return false;
+				}
+			},
+			node.get_value()
+		);

	// TODO: implement
	spdlog::warn_s("ProvinceInstance::evaluate_leaf is not yet implemented; returning false for condition.");

Conversation

DarkiBoi commented Feb 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

wvpm left a comment

Choose a reason for hiding this comment

Uh oh!

wvpm Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments