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 };

@DarkiBoi A state doesn't even have an identifier.
Consider skipping states for now.
I think we'll use Regions later on either as scope or merely for the identifier.

Terminology:
Region Victoria 2\map\regions.txt, for example Texas.
State a part of a region owned by a country with a colonial status. For example American colonial Texas.

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.

src/openvic-simulation/country/CountryInstance.cpp

src/openvic-simulation/scripts/Condition.cpp

src/openvic-simulation/scripts/Condition.hpp

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.

src/openvic-simulation/scripts/Condition.cpp

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()

);

src/openvic-simulation/map/ProvinceInstance.cpp

src/openvic-simulation/scripts/Condition.cpp

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.

src/openvic-simulation/country/CountryInstance.cpp

also vacuous truth fix

wvpm · 2026-02-24T08:10:50Z

src/openvic-simulation/country/CountryInstance.hpp

 	struct TechnologySchool;
 	struct UnitInstanceGroup;
 	struct UnitTypeManager;
+	struct ConditionNode;


Suggested change

struct ConditionNode;

No longer needed

wvpm · 2026-02-24T08:11:04Z

src/openvic-simulation/map/State.hpp

 	struct StateManager;
 	struct StateSet;
 	struct Strata;
+	struct ConditionNode;


Suggested change

struct ConditionNode;

wvpm · 2026-02-24T08:11:25Z

src/openvic-simulation/population/Pop.cpp

 #include "openvic-simulation/types/TypedIndices.hpp"
 #include "openvic-simulation/utility/Containers.hpp"
 #include "openvic-simulation/utility/Logger.hpp"
+#include "openvic-simulation/core/Typedefs.hpp"


Suggested change

#include "openvic-simulation/core/Typedefs.hpp"

Not related to this PR

wvpm · 2026-02-24T08:11:33Z

src/openvic-simulation/population/Pop.hpp

 	struct RebelType;
 	struct Religion;
 	struct SellResult;
+	struct ConditionNode;


Suggested change

struct ConditionNode;

wvpm · 2026-02-24T08:13:58Z

src/openvic-simulation/scripts/Context.hpp

+			auto* p,
+			DefinitionManager const& dm,
+			InstanceManager const& im,
+			Context const* this_ctx,


The naming convention inside this project is new_[field_name] for constructor arguments.
That also avoids a distinction between scope and context.

wvpm · 2026-02-24T08:16:14Z

src/openvic-simulation/scripts/Context.hpp

+		Context const* from_scope = nullptr;
+
+		Context(CountryInstance const* p, DefinitionManager const& dm, InstanceManager const& im)
+			: ptr(p), definition_manager(dm), instance_manager(im), this_scope(this) {}


When using constructor overloads, forward the arguments to the main constructor (the line 39 one I think).
This ensures all constructors function the same and the overloads are merely for convenience.

wvpm · 2026-02-24T08:20:08Z

src/openvic-simulation/scripts/Context.hpp

+	struct DefinitionManager;
+	struct InstanceManager;
+
+	struct Context {


The default access modifier of a struct is public.
In this project we avoid mutable publci fields.

You could make the fields private and expose their getters via the PROPERTY macro.
If the fields are only set in the constructor, you have to write Context const* const.
The const* makes it a pointer to a readonly Context. The second const makes the pointer itself readonly.

DefinitionManager const& definition_manager; & InstanceManager const& instance_manager; can safely be exposed as public fields as references are readonly.

wvpm · 2026-02-24T08:21:35Z

src/openvic-simulation/scripts/Context.hpp

+		Context(Pop const* p, DefinitionManager const& dm, InstanceManager const& im)
+			: ptr(p), definition_manager(dm), instance_manager(im), this_scope(this) {}
+
+		Context(


Consider making this constructor private unless required otherwise

wvpm · 2026-02-24T08:23:59Z

src/openvic-simulation/scripts/Context.hpp

+
+#include <optional>
+#include <variant>
+#include <vector>


Spartan322 added memory tracking. This requires using memory::vector instead of std::vector.
See also #421
So use #include "openvic-simulation/utility/Containers.hpp" instead.

wvpm · 2026-02-24T08:26:22Z

src/openvic-simulation/scripts/Context.hpp

+		DefinitionManager const& definition_manager;
+		InstanceManager const& instance_manager;
+
+		Context const* this_scope = nullptr;


Can THIS ever be null?
How is accessing context.this_scope different from just using context?

wvpm · 2026-02-24T08:28:27Z

src/openvic-simulation/scripts/Context.cpp

+#include "openvic-simulation/InstanceManager.hpp"
+
+using namespace OpenVic;
+scope_type_t Context::get_scope_type() const {


Why not determine this during construction and store it as a field?

wvpm · 2026-02-24T08:35:31Z

src/openvic-simulation/scripts/Context.cpp

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


For debug purposes, I suggest you at least write the pop type here.
Alternatively use pop.id_in_province & pop.get_location()->index.

wvpm · 2026-02-24T08:37:53Z

src/openvic-simulation/scripts/Context.cpp

+			using T = std::decay_t<decltype(*p)>;
+
+			if constexpr (std::is_same_v<T, CountryInstance>) {
+				// TODO: https://vic2.paradoxwikis.com/List_of_conditions#Country_Scope


Please move the body of these if statements to separate methods.

if constexpr (std::is_same_v<T, CountryInstance>) { evaluate_country_leaf(node, *p); }

bool evaluate_country_leaf(ConditionNode const& node, CountryInstance const& country) const;

wvpm · 2026-02-24T08:40:42Z

src/openvic-simulation/scripts/Context.cpp

+bool Context::evaluate_leaf(ConditionNode const& node) const {
+	std::string_view const id = node.get_condition()->get_identifier();
+
+	return std::visit(


This isn't how std::visit is designed. See https://en.cppreference.com/w/cpp/utility/variant/visit
Either use if constexpr (std::is_same_v<T, CountryInstance>) or write overloads.

wvpm · 2026-02-24T08:45:26Z

src/openvic-simulation/scripts/Context.cpp

+	return result;
+}
+
+std::optional<Context> Context::get_redirect_context(std::string_view condition_id, scope_type_t target) const {


std::optional<Context> owns the Context.
Both this_scope and from_scope already exist and are owned somewhere else.
Returning them here copies them. I don't think that's what you want.

wvpm · 2026-02-24T08:47:05Z

src/openvic-simulation/scripts/Context.cpp

+
+		if (target == scope_type_t::COUNTRY) {
+			if (condition_id == "owner") {
+				auto const* owner = province->get_owner();


In this project explicit types are preferred over auto types.
Just use CountryInstance const* owner here and for controller.

wvpm · 2026-02-24T08:49:47Z

src/openvic-simulation/scripts/Context.cpp

+				return false;
+			}
+			else if constexpr (std::is_same_v<T, State>) {
+				// No state conditions according to wiki?


See Victoria 2\common\production_types.txt
trade_goods_in_state = coal
has_building = electric_gear_factory

wvpm · 2026-02-24T08:54:55Z

src/openvic-simulation/scripts/Context.cpp

+				}
+				if (id == "badboy") {
+					fixed_point_t expected_ratio = std::get<fixed_point_t>(node.get_value());
+					return p->get_infamy_untracked() >= expected_ratio * definition_manager.get_define_manager().get_country_defines().get_infamy_containment_limit();


If you only need this define, you should request the CountryDefines in your constructor instead of searching for it yourself. Same for the CountryInstanceManager.

We can leave this as is and trim it later once other conditions are implemented.

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

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

src/openvic-simulation/country/CountryInstance.cpp Outdated Show resolved Hide resolved

DarkiBoi force-pushed the conditions branch 2 times, most recently from d553f4f to 5c25897 Compare February 22, 2026 17:24

condition evaluation wip

d953656

DarkiBoi force-pushed the conditions branch from 5c25897 to d953656 Compare February 23, 2026 18:18

DarkiBoi added 7 commits February 23, 2026 19:25

remove overloading

0338a76

fix any_core duplicate

16e1429

fix war_countries not being an iterator

08bc046

also vacuous truth fix

move evaluate_leaf out of the types

2423a90

null check

d1515c1

let context access definition manager

1f555ab

implement exists properly

12b8b03

wvpm reviewed Feb 24, 2026

View reviewed changes

Spartan322 added the enhancement New feature or request label Feb 24, 2026

-			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 };

		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()
+		);

Comments

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!

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!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wvpm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvpm Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvpm Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvpm Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

wvpm Feb 24, 2026 •

edited

Loading

wvpm Feb 24, 2026 •

edited

Loading

wvpm Feb 24, 2026 •

edited

Loading