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.

Author: willvale

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

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;

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<Command>(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<value_type::divert>(), true,
-					    ! (flag & CommandFlag::DIVERT_HAS_CONDITION)
+					    _story->instructions() + val->get<value_type::divert>(), 
+							record_visits, track_knot_visit
 					);
 					inkAssert(_ptr < _story->end(), "Diverted past end of story data!");
 				} break;

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");