From cfb8c82b527e2af0b4293a640134f33810d8bb33 Mon Sep 17 00:00:00 2001 From: willvale <66674079+willvale@users.noreply.github.com> Date: Mon, 29 Jun 2026 20:57:21 +1200 Subject: [PATCH 1/6] Fix knot tracking for tunnels and threads Fix knot tracking for tunnels and threads Knots, tunnels and threads should all have entry tracked, otherwise they don't get per-knot tags. * Set track_not_visit for all frames other than functions. * Added new test TagsAndBranching which tests that tags end up visible in inkcpp for all knot types. --- inkcpp/runner_impl.cpp | 10 ++- inkcpp_test/CMakeLists.txt | 1 + inkcpp_test/TagsAndBranching.cpp | 96 ++++++++++++++++++++++++++++ inkcpp_test/ink/TagsAndBranching.ink | 23 +++++++ 4 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 inkcpp_test/TagsAndBranching.cpp create mode 100644 inkcpp_test/ink/TagsAndBranching.ink diff --git a/inkcpp/runner_impl.cpp b/inkcpp/runner_impl.cpp index bb3bec2f..652d8a5e 100644 --- a/inkcpp/runner_impl.cpp +++ b/inkcpp/runner_impl.cpp @@ -425,9 +425,12 @@ void runner_impl::start_frame(uint32_t target) } _evaluation_mode = false; // unset eval mode when enter function or tunnel + // Do we visit the knot? In Ink, all visits count for e.g. knot tags. + const bool track_knot_visit = type == frame_type::function; + // Do the jump inkAssert(_story->instructions() + target < _story->end(), "Diverting past end of story data!"); - jump(_story->instructions() + target, true, false); + jump(_story->instructions() + target, true, track_knot_visit); } frame_type runner_impl::execute_return() @@ -461,13 +464,16 @@ frame_type runner_impl::execute_return() } } + // Do we visit the knot? In Ink, all visits count for e.g. knot tags. Since we tracked the visit + // when entering the thread or tunnel, we need to track the return to where we came from. + const bool track_knot_visit = type == frame_type::function; // Jump to the old offset inkAssert( _story->instructions() + offset < _story->end(), "Callstack return is outside bounds of story!" ); - jump(_story->instructions() + offset, false, false); + jump(_story->instructions() + offset, false, track_knot_visit); // Return frame type return type; diff --git a/inkcpp_test/CMakeLists.txt b/inkcpp_test/CMakeLists.txt index 95b757ef..6e8cbd5b 100644 --- a/inkcpp_test/CMakeLists.txt +++ b/inkcpp_test/CMakeLists.txt @@ -15,6 +15,7 @@ add_executable( Globals.cpp Lists.cpp Tags.cpp + TagsAndBranching.cpp NewLines.cpp FallbackFunction.cpp LabelCondition.cpp diff --git a/inkcpp_test/TagsAndBranching.cpp b/inkcpp_test/TagsAndBranching.cpp new file mode 100644 index 00000000..205289b9 --- /dev/null +++ b/inkcpp_test/TagsAndBranching.cpp @@ -0,0 +1,96 @@ +#include "catch.hpp" +#include "system.h" + +#include <../runner_impl.h> +#include +#include +#include +#include +#include + +using namespace ink::runtime; + +SCENARIO("TagsAndBranching", "[tags][branching]") +{ + GIVEN("A story with tags and branching") + { + story* _ink = story::from_file(INK_TEST_RESOURCE_DIR "TagsAndBranching.bin"); + runner _thread = _ink->new_runner(); + + WHEN("Starting the thread") + { + CHECK_FALSE(_thread->has_tags()); + CHECK_FALSE(_thread->has_knot_tags()); + CHECK(_thread->get_current_knot() == 0); + } + + WHEN("On the plain text line") + { + CHECK(_thread->getline() == "Plain text\n"); + THEN("It has tags") + { + CHECK(!_thread->has_knot_tags()); + CHECK(_thread->has_tags()); + REQUIRE(_thread->num_tags() == 1); + REQUIRE(std::string(_thread->get_tag(0)) == "plain_text_tag"); + } + } + + WHEN("In the knot") + { + // Skip previous test + _thread->getline(); + CHECK(_thread->getline() == "Knot text\n"); + THEN("It has tags") + { + CHECK(_thread->get_current_knot() == ink::hash_string("Knot")); + CHECK(_thread->has_knot_tags()); + REQUIRE(_thread->num_knot_tags() == 1); + REQUIRE(std::string(_thread->get_knot_tag(0)) == "knot_tag"); + CHECK(_thread->has_tags()); + REQUIRE(_thread->num_tags() == 2); + REQUIRE(std::string(_thread->get_tag(0)) == "knot_tag"); + REQUIRE(std::string(_thread->get_tag(1)) == "knot_text_tag"); + } + } + + WHEN("In the tunnel") + { + // Skip previous tests + _thread->getline(); + _thread->getline(); + CHECK(_thread->getline() == "Tunnel text\n"); + THEN("It has tags") + { +// CHECK(_thread->get_current_knot() == ink::hash_string("Tunnel")); + CHECK(_thread->has_knot_tags()); + REQUIRE(_thread->num_knot_tags() == 1); + REQUIRE(std::string(_thread->get_knot_tag(0)) == "tunnel_tag"); + CHECK(_thread->has_tags()); + REQUIRE(_thread->num_tags() == 2); + REQUIRE(std::string(_thread->get_tag(0)) == "tunnel_tag"); + REQUIRE(std::string(_thread->get_tag(1)) == "tunnel_text_tag"); + } + } + + WHEN("In the thread") + { + // Skip previous tests + _thread->getline(); + _thread->getline(); + _thread->getline(); + CHECK(_thread->getline() == "Thread text\n"); + THEN("It has tags") + { + CHECK(_thread->get_current_knot() == ink::hash_string("Thread")); + CHECK(_thread->has_knot_tags()); + REQUIRE(_thread->num_knot_tags() == 1); + REQUIRE(std::string(_thread->get_knot_tag(0)) == "thread_tag"); + CHECK(_thread->has_tags()); + REQUIRE(_thread->num_tags() == 2); + REQUIRE(std::string(_thread->get_tag(0)) == "thread_tag"); + REQUIRE(std::string(_thread->get_tag(1)) == "thread_text_tag"); + } + } + } +} diff --git a/inkcpp_test/ink/TagsAndBranching.ink b/inkcpp_test/ink/TagsAndBranching.ink new file mode 100644 index 00000000..d32882ad --- /dev/null +++ b/inkcpp_test/ink/TagsAndBranching.ink @@ -0,0 +1,23 @@ +Plain text # plain_text_tag +->Knot + +=== Continue +->Tunnel-> +<-Thread +->DONE + +// All these knots should be tracked for tagging and visit counts +=== Knot + #knot_tag + Knot text #knot_text_tag + ->Continue + +=== Tunnel + #tunnel_tag + Tunnel text #tunnel_text_tag + ->-> + +=== Thread + #thread_tag + Thread text #thread_text_tag + ->DONE From a4c64172a9c1798d701842cb4b32b9870446b425 Mon Sep 17 00:00:00 2001 From: willvale <66674079+willvale@users.noreply.github.com> Date: Mon, 29 Jun 2026 23:37:40 +1200 Subject: [PATCH 2/6] Maybe fix Clangformat issues --- inkcpp_test/TagsAndBranching.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/inkcpp_test/TagsAndBranching.cpp b/inkcpp_test/TagsAndBranching.cpp index 205289b9..ac79c66c 100644 --- a/inkcpp_test/TagsAndBranching.cpp +++ b/inkcpp_test/TagsAndBranching.cpp @@ -29,7 +29,7 @@ SCENARIO("TagsAndBranching", "[tags][branching]") CHECK(_thread->getline() == "Plain text\n"); THEN("It has tags") { - CHECK(!_thread->has_knot_tags()); + CHECK(! _thread->has_knot_tags()); CHECK(_thread->has_tags()); REQUIRE(_thread->num_tags() == 1); REQUIRE(std::string(_thread->get_tag(0)) == "plain_text_tag"); @@ -62,7 +62,8 @@ SCENARIO("TagsAndBranching", "[tags][branching]") CHECK(_thread->getline() == "Tunnel text\n"); THEN("It has tags") { -// CHECK(_thread->get_current_knot() == ink::hash_string("Tunnel")); + // This doesn't pass yet, not sure why. + // CHECK(_thread->get_current_knot() == ink::hash_string("Tunnel")); CHECK(_thread->has_knot_tags()); REQUIRE(_thread->num_knot_tags() == 1); REQUIRE(std::string(_thread->get_knot_tag(0)) == "tunnel_tag"); From 2f5c1a89c121e04abbb532caebc244d673ac0b1a Mon Sep 17 00:00:00 2001 From: willvale <66674079+willvale@users.noreply.github.com> Date: Mon, 29 Jun 2026 23:40:57 +1200 Subject: [PATCH 3/6] Another rogue CR --- inkcpp_test/TagsAndBranching.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/inkcpp_test/TagsAndBranching.cpp b/inkcpp_test/TagsAndBranching.cpp index ac79c66c..c0cfd54a 100644 --- a/inkcpp_test/TagsAndBranching.cpp +++ b/inkcpp_test/TagsAndBranching.cpp @@ -62,7 +62,7 @@ SCENARIO("TagsAndBranching", "[tags][branching]") CHECK(_thread->getline() == "Tunnel text\n"); THEN("It has tags") { - // This doesn't pass yet, not sure why. + // This doesn't pass yet, not sure why. // CHECK(_thread->get_current_knot() == ink::hash_string("Tunnel")); CHECK(_thread->has_knot_tags()); REQUIRE(_thread->num_knot_tags() == 1); From 7376829f430bd801e2b3ed9c518fafe518a250f4 Mon Sep 17 00:00:00 2001 From: willvale <66674079+willvale@users.noreply.github.com> Date: Fri, 3 Jul 2026 11:03:04 +1200 Subject: [PATCH 4/6] Fix typo in tracking selection Track non-functions on jump, don't update counts on return. Also: * Remove parameter from visit() which makes it do nothing, just don't call it. * Add named Booleans for everywhere we call jump - 3 parameters of the same type with default values is easy to get wrong otherwise. In future we should change this to a set of flags, but I want to see if the logic can be cleaned up a bit first. It's fiddly. --- inkcpp/globals_impl.cpp | 6 +-- inkcpp/globals_impl.h | 4 +- inkcpp/runner_impl.cpp | 63 ++++++++++++++++++++++---------- inkcpp_test/TagsAndBranching.cpp | 3 -- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/inkcpp/globals_impl.cpp b/inkcpp/globals_impl.cpp index 40b7eedf..e4d8c48c 100644 --- a/inkcpp/globals_impl.cpp +++ b/inkcpp/globals_impl.cpp @@ -30,12 +30,10 @@ globals_impl::globals_impl(const story_impl* story) } } -void globals_impl::visit(uint32_t container_id, bool preserve_turns) +void globals_impl::visit(uint32_t container_id) { - const int32_t existing_turns = _visit_counts[container_id].turns; _visit_counts.set( - container_id, {_visit_counts[container_id].visits + (preserve_turns ? 0 : 1), - preserve_turns ? existing_turns : 0} + container_id, {_visit_counts[container_id].visits + 1, 0} ); } diff --git a/inkcpp/globals_impl.h b/inkcpp/globals_impl.h index 854a3ec8..5e67597e 100644 --- a/inkcpp/globals_impl.h +++ b/inkcpp/globals_impl.h @@ -65,9 +65,7 @@ class globals_impl final public: // Records a visit to a container. - // If preserve_turns is true the existing turns-since counter is kept intact - // (used during snapshot migration to avoid clobbering the restored value). - void visit(uint32_t container_id, bool preserve_turns = false); + void visit(uint32_t container_id); // Checks the number of visits to a container uint32_t visits(uint32_t container_id) const; diff --git a/inkcpp/runner_impl.cpp b/inkcpp/runner_impl.cpp index 652d8a5e..6b53932c 100644 --- a/inkcpp/runner_impl.cpp +++ b/inkcpp/runner_impl.cpp @@ -354,8 +354,8 @@ void runner_impl::jump(ip_t dest, bool record_visits, bool track_knot_visit, boo const container_data_t& dest_container = _story->container_data(dest_id); if (dest_offset == dest_container._start_offset) { // Record direct jump to non-knot if requested. (Knots handled below.) - if (record_visits && ! dest_container.knot()) { - _globals->visit(dest_id, preserve_turns); + if (! preserve_turns && record_visits && ! dest_container.knot()) { + _globals->visit(dest_id); } // Consume instruction so we don't process it again during normal flow. (We need to do this here @@ -387,8 +387,8 @@ void runner_impl::jump(ip_t dest, bool record_visits, bool track_knot_visit, boo // // Ink has a rule about incrementing visit counts when you jump to the top of a knot, which // seems to need to override inkcpp's knot_visit flag. - if (track_knot_visit || container._start_offset == dest_offset) { - _globals->visit(id, preserve_turns); + if (! preserve_turns && (track_knot_visit || container._start_offset == dest_offset)) { + _globals->visit(id); } // If tracking, update with the first knot we encounter, which is the one closest to the top @@ -425,12 +425,15 @@ void runner_impl::start_frame(uint32_t target) } _evaluation_mode = false; // unset eval mode when enter function or tunnel - // Do we visit the knot? In Ink, all visits count for e.g. knot tags. - const bool track_knot_visit = type == frame_type::function; + // Always record visits + const bool record_visits = true; + + // Do we visit the knot? We need to visit anything that can have knot tags. + const bool track_knot_visit = type != frame_type::function; // Do the jump inkAssert(_story->instructions() + target < _story->end(), "Diverting past end of story data!"); - jump(_story->instructions() + target, true, track_knot_visit); + jump(_story->instructions() + target, record_visits, track_knot_visit); } frame_type runner_impl::execute_return() @@ -464,16 +467,21 @@ frame_type runner_impl::execute_return() } } - // Do we visit the knot? In Ink, all visits count for e.g. knot tags. Since we tracked the visit - // when entering the thread or tunnel, we need to track the return to where we came from. - const bool track_knot_visit = type == frame_type::function; + // Never record visits + const bool record_visits = false; + + // Do we visit the knot? This needs to match what we tracked in start_frame. + const bool track_knot_visit = type != frame_type::function; + + // Returns should never update visit counts. + const bool preserve_turns = true; // Jump to the old offset inkAssert( _story->instructions() + offset < _story->end(), "Callstack return is outside bounds of story!" ); - jump(_story->instructions() + offset, false, track_knot_visit); + jump(_story->instructions() + offset, record_visits, track_knot_visit, preserve_turns); // Return frame type return type; @@ -631,7 +639,11 @@ void runner_impl::choose(size_t index) inkAssert(prev != nullptr, "No 'done' point recorded before finishing choice output"); // Move to the previous pointer so we track our movements correctly - jump(prev, false, false); + { + const bool record_visits = false; + const bool track_knot_visit = false; + jump(prev, record_visits, track_knot_visit); + } _done = nullptr; // Collapse callstacks to the correct thread @@ -641,7 +653,9 @@ void runner_impl::choose(size_t index) _eval.clear(); // Jump to destination and clear choice list - jump(_story->instructions() + c.path(), true, false); + const bool record_visits = true; + const bool track_knot_visit = false; + jump(_story->instructions() + c.path(), record_visits, track_knot_visit); clear_choices(); _entered_knot = false; } @@ -821,7 +835,9 @@ bool runner_impl::move_to(hash_t path) // Clear state and move to destination reset(); _ptr = _story->instructions(); - jump(destination, false, false); + const bool record_visits = false; + const bool track_knot_visit = false; + jump(destination, record_visits, track_knot_visit); return true; } @@ -852,7 +868,9 @@ bool runner_impl::migrate_to(const loader& loader, hash_t path) while (read(eval_start) != Command::START_EVAL) { eval_start -= 6; } - jump(eval_start, false, false); + const bool record_visits = false; + const bool track_knot_visit = false; + jump(eval_start, record_visits, track_knot_visit); while (_ptr != iter + 6) { step(); } @@ -866,7 +884,10 @@ bool runner_impl::migrate_to(const loader& loader, hash_t path) // without this the visit() call inside jump() would reset them to 0. _container.clear(); _ptr = nullptr; - jump(destination, false, true, true); + const bool record_visits = false; + const bool track_knot_visit = false; + const bool preserve_turns = true; + jump(destination, record_visits, track_knot_visit, preserve_turns); if (loader.old_ref_table && ! _globals->lists().migrate_variables( @@ -1208,7 +1229,9 @@ void runner_impl::step() inkAssert( _story->instructions() + target < _story->end(), "Diverting past end of story data!" ); - jump(_story->instructions() + target, true, ! (flag & CommandFlag::DIVERT_HAS_CONDITION)); + const bool record_visits = true; + const bool track_knot_visit = ! (flag & CommandFlag::DIVERT_HAS_CONDITION); + jump(_story->instructions() + target, record_visits, track_knot_visit); } break; case Command::DIVERT_TO_VARIABLE: { // Get variable value @@ -1230,9 +1253,11 @@ void runner_impl::step() inkAssert(val, "Jump destiniation needs to be defined!"); // Move to location + const bool record_visits = true; + const bool track_knot_visit = ! (flag & CommandFlag::DIVERT_HAS_CONDITION); jump( - _story->instructions() + val->get(), true, - ! (flag & CommandFlag::DIVERT_HAS_CONDITION) + _story->instructions() + val->get(), + record_visits, track_knot_visit ); inkAssert(_ptr < _story->end(), "Diverted past end of story data!"); } break; diff --git a/inkcpp_test/TagsAndBranching.cpp b/inkcpp_test/TagsAndBranching.cpp index c0cfd54a..8a42bf96 100644 --- a/inkcpp_test/TagsAndBranching.cpp +++ b/inkcpp_test/TagsAndBranching.cpp @@ -62,8 +62,6 @@ SCENARIO("TagsAndBranching", "[tags][branching]") CHECK(_thread->getline() == "Tunnel text\n"); THEN("It has tags") { - // This doesn't pass yet, not sure why. - // CHECK(_thread->get_current_knot() == ink::hash_string("Tunnel")); CHECK(_thread->has_knot_tags()); REQUIRE(_thread->num_knot_tags() == 1); REQUIRE(std::string(_thread->get_knot_tag(0)) == "tunnel_tag"); @@ -83,7 +81,6 @@ SCENARIO("TagsAndBranching", "[tags][branching]") CHECK(_thread->getline() == "Thread text\n"); THEN("It has tags") { - CHECK(_thread->get_current_knot() == ink::hash_string("Thread")); CHECK(_thread->has_knot_tags()); REQUIRE(_thread->num_knot_tags() == 1); REQUIRE(std::string(_thread->get_knot_tag(0)) == "thread_tag"); From bd77f52cfed74815121627a8fcfc64c5afd28537 Mon Sep 17 00:00:00 2001 From: willvale <66674079+willvale@users.noreply.github.com> Date: Fri, 3 Jul 2026 11:20:11 +1200 Subject: [PATCH 5/6] Manual ClangFormat fixes Can't just run ClangFormat in VS as that makes further changes unrelated to the code changes. --- inkcpp/globals_impl.cpp | 4 +--- inkcpp/runner_impl.cpp | 6 +++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/inkcpp/globals_impl.cpp b/inkcpp/globals_impl.cpp index e4d8c48c..17be9185 100644 --- a/inkcpp/globals_impl.cpp +++ b/inkcpp/globals_impl.cpp @@ -32,9 +32,7 @@ globals_impl::globals_impl(const story_impl* story) void globals_impl::visit(uint32_t container_id) { - _visit_counts.set( - container_id, {_visit_counts[container_id].visits + 1, 0} - ); + _visit_counts.set(container_id, {_visit_counts[container_id].visits + 1, 0}); } uint32_t globals_impl::visits(uint32_t container_id) const diff --git a/inkcpp/runner_impl.cpp b/inkcpp/runner_impl.cpp index 6b53932c..651c9b4b 100644 --- a/inkcpp/runner_impl.cpp +++ b/inkcpp/runner_impl.cpp @@ -835,7 +835,7 @@ bool runner_impl::move_to(hash_t path) // Clear state and move to destination reset(); _ptr = _story->instructions(); - const bool record_visits = false; + const bool record_visits = false; const bool track_knot_visit = false; jump(destination, record_visits, track_knot_visit); @@ -1256,8 +1256,8 @@ void runner_impl::step() const bool record_visits = true; const bool track_knot_visit = ! (flag & CommandFlag::DIVERT_HAS_CONDITION); jump( - _story->instructions() + val->get(), - record_visits, track_knot_visit + _story->instructions() + val->get(), record_visits, + track_knot_visit ); inkAssert(_ptr < _story->end(), "Diverted past end of story data!"); } break; From 797609468c66294506d768c9a8376f71d7730542 Mon Sep 17 00:00:00 2001 From: willvale <66674079+willvale@users.noreply.github.com> Date: Fri, 3 Jul 2026 13:25:34 +1200 Subject: [PATCH 6/6] Only look for migratable locals when the destination is inside the current knot The knot-recording change for tags means that this isn't always true. And if the destination is behind, the scanning code eats the entire story. --- inkcpp/runner_impl.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/inkcpp/runner_impl.cpp b/inkcpp/runner_impl.cpp index 651c9b4b..c987311a 100644 --- a/inkcpp/runner_impl.cpp +++ b/inkcpp/runner_impl.cpp @@ -855,7 +855,8 @@ bool runner_impl::migrate_to(const loader& loader, hash_t path) ip_t start_of_knot = _story->find_offset_for(_story->container_data(_current_knot_id)._hash); fetch_tags(start_of_knot); assign_tags({tags_level::KNOT}); - if (start_of_knot != destination) { + // If the destination is in the recorded current knot + if (start_of_knot < destination) { for (ip_t iter = start_of_knot; iter != destination; iter += 6) { if (read(iter) == Command::DEFINE_TEMP) { hash_t temp_name = read(iter + 2);