fix(runner): count knot visits after jump more correctly

inkcpp/runner_impl.cpp

@@ -16,6 +16,7 @@
 #include "value.h"
 
 #include <iomanip>
+#include <vector>
 
 namespace ink::runtime
 {
@@ -42,7 +43,7 @@ namespace ink::runtime::internal
 
 hash_t runner_impl::get_current_knot() const
 {
-	return _current_knot_id == ~0 ? 0 : _story->container_hash(_current_knot_id);
+	return _current_knot_id == ~0U ? 0 : _story->container_hash(_current_knot_id);
 }
 
 template<>
@@ -298,104 +299,98 @@ void runner_impl::jump(ip_t dest, bool record_visits, bool track_knot_visit)
 	//  _should be_ able to safely assume that there is nothing to do here. A falling
 	//  divert should only be taking us from a container to that same container's end point
 	//  without entering any other containers
-	// OR IF if target is same position do nothing
-	// could happend if jumping to and of an unnamed container
+	// OR IF is target is same position do nothing
+	// could happened if jumping to and of an unnamed container
 	if (dest == _ptr) {
 		_ptr = dest;
 		return;
 	}
 
 	const uint32_t* iter = nullptr;
 	container_t     id;
-	ip_t            offset = nullptr;
-	size_t          comm_end;
+	ip_t            offset   = nullptr;
 	bool            reversed = _ptr > dest;
 
-	if (reversed) {
-		comm_end                            = 0;
-		iter                                = nullptr;
-		const ContainerData* old_iter       = nullptr;
-		const uint32_t*      last_comm_iter = nullptr;
-		_container.rev_iter(old_iter);
-
-		// find commen part of old and new stack
-		while (_story->iterate_containers(iter, id, offset)) {
-			if (old_iter == nullptr || offset >= dest) {
-				break;
-			}
-			if (old_iter != nullptr && id == old_iter->id) {
-				last_comm_iter = iter;
-				_container.rev_iter(old_iter);
-				++comm_end;
-			}
-		}
-
-		// clear old part from stack
-		while (_container.size() > comm_end) {
-			_container.pop();
-		}
-		iter = last_comm_iter;
+	// move to destination and update container stack on the go
+	const ContainerData* c_iter   = nullptr;
+	// number of container which were already on the stack at current position
+	size_t               comm_end = _container.size();
 
-	} else {
-		iter     = nullptr;
-		comm_end = _container.size();
-		// go to current possition in container list
-		while (_story->iterate_containers(iter, id, offset)) {
-			if (offset >= _ptr) {
-				break;
-			}
+	iter = nullptr;
+	while (_story->iterate_containers(iter, id, offset)) {
+		if (offset >= _ptr) {
+			break;
 		}
+	}
+	if (! reversed) {
 		_story->iterate_containers(iter, id, offset, true);
 	}
-
-	// move to destination and update container stack on the go
-	while (_story->iterate_containers(iter, id, offset)) {
-		if (offset >= dest) {
+	optional<ContainerData> last_pop = nullopt;
+	while (_story->iterate_containers(iter, id, offset, reversed)) {
+		if ((! reversed && offset >= dest) || (reversed && offset < dest)) {
 			break;
 		}
 		if (_container.empty() || _container.top().id != id) {
-			_container.push({id, offset - _story->instructions()});
+			const uint32_t* iter2 = nullptr;
+			container_t     id2;
+			ip_t            offset2;
+			while (_story->iterate_containers(iter2, id2, offset2) && id2 != id) {}
+			_container.push({id, offset2 - _story->instructions()});
 		} else {
-			_container.pop();
-			if (_container.size() < comm_end) {
-				comm_end = _container.size();
+			if (_container.size() == comm_end) {
+				last_pop = _container.pop();
+				comm_end -= 1;
+			} else {
+				_container.pop();
 			}
 		}
 	}
-	_ptr = dest;
+	iter = nullptr;
+	while (_story->iterate_containers(iter, id, offset)) {
+		if (offset >= dest) {
+			break;
+		}
+	}
 
-	// if we jump directly to a named container start, go inside, if its a ONLY_FIRST container
+	// if we jump directly to a named container start, go inside, if it's a ONLY_FIRST container
 	// it will get visited in the next step
+	// todo: check if a while is needed
 	if (offset == dest && static_cast<Command>(offset[0]) == Command::START_CONTAINER_MARKER) {
 		if (track_knot_visit
 		    && static_cast<CommandFlag>(offset[1]) & CommandFlag::CONTAINER_MARKER_IS_KNOT) {
 			_current_knot_id = id;
 			_entered_knot    = true;
 		}
-		_ptr += 6;
+		dest += 6;
 		_container.push({id, offset - _story->instructions()});
-		if (reversed && comm_end == _container.size() - 1) {
-			++comm_end;
+		// if we entered a knot we just left, do not recount enter
+		if (reversed && comm_end == _container.size() - 1 && last_pop.has_value()
+		    && last_pop.value().id == id) {
+			comm_end += 1;
 		}
 	}
+	_ptr = dest;
 
 	// iff all container (until now) are entered at first position
 	bool allEnteredAtStart = true;
 	ip_t child_position    = dest;
 	if (record_visits) {
 		const ContainerData* iData = nullptr;
 		size_t               level = _container.size();
-		if (_container.iter(iData)
-		    && (level > comm_end
-		        || _story->container_flag(iData->offset + _story->instructions())
-		               & CommandFlag::CONTAINER_MARKER_ONLY_FIRST)) {
-			auto parrent_offset = _story->instructions() + iData->offset;
-			inkAssert(child_position >= parrent_offset, "Container stack order is broken");
-			// 6 == len of START_CONTAINER_SIGNAL, if its 6 bytes behind the container it is a unnnamed
-			// subcontainers first child check if child_positino is the first child of current container
-			allEnteredAtStart = allEnteredAtStart && ((child_position - parrent_offset) <= 6);
-			child_position    = parrent_offset;
-			_globals->visit(iData->id, allEnteredAtStart);
+		while (_container.iter(iData)) {
+			if (level > comm_end
+			    || _story->container_flag(iData->offset + _story->instructions())
+			           & CommandFlag::CONTAINER_MARKER_ONLY_FIRST) {
+				auto parrent_offset = _story->instructions() + iData->offset;
+				inkAssert(child_position >= parrent_offset, "Container stack order is broken");
+				// 6 == len of START_CONTAINER_SIGNAL, if its 6 bytes behind the container it is a
+				// unnnamed subcontainers first child check if child_positino is the first child of
+				// current container
+				allEnteredAtStart = allEnteredAtStart && ((child_position - parrent_offset) <= 6);
+				child_position    = parrent_offset;
+				_globals->visit(iData->id, allEnteredAtStart);
+			}
+			level -= 1;
 		}
 	}
 }
@@ -512,13 +507,13 @@ runner_impl::line_type runner_impl::getline()
 	// Advance interpreter one line and write to output
 	advance_line();
 
-#ifdef INK_ENABLE_STL
+#	ifdef INK_ENABLE_STL
 	line_type result{_output.get()};
-#elif defined(INK_ENABLE_UNREAL)
+#	elif defined(INK_ENABLE_UNREAL)
 	line_type result{ANSI_TO_TCHAR(_output.get_alloc(_globals->strings(), _globals->lists()))};
-#else
-#	error unsupported constraints for getline
-#endif
+#	else
+#		error unsupported constraints for getline
+#	endif
 
 	// Fall through the fallback choice, if available
 	if (! has_choices() && _fallback_choice) {
@@ -531,11 +526,11 @@ runner_impl::line_type runner_impl::getline()
 
 runner_impl::line_type runner_impl::getall()
 {
-#ifdef INK_ENABLE_STL
+#	ifdef INK_ENABLE_STL
 	if (_debug_stream != nullptr) {
 		_debug_stream->clear();
 	}
-#endif
+#	endif
 
 	line_type result{};
 
@@ -1385,7 +1380,7 @@ void runner_impl::step()
 						current_choice = &add_choice();
 					}
 					current_choice->setup(
-					    _output, _globals->strings(), _globals->lists(), _choices.size(), path,
+					    _output, _globals->strings(), _globals->lists(), _choices.size() - 1, path,
 					    current_thread(), tags_start, tags_end
 					);
 					// save stack at last choice

inkcpp_compiler/binary_emitter.cpp

@@ -361,7 +361,7 @@ void binary_emitter::process_paths()
 			token = ink::compiler::internal::strtok_s(nullptr, ".", &_context);
 		}
 
-		if (noop_offset != ~0) {
+		if (noop_offset != ~0U) {
 			inkAssert(! useCountIndex, "Can't count visits to a noop!");
 			_containers.set(position, noop_offset);
 		} else {

inkcpp_compiler/json_compiler.cpp

@@ -139,17 +139,17 @@ void json_compiler::compile_container(
 	if (is_knot) {
 		// it is not a wave or choice
 		if (::ink::internal::starts_with(name_override.c_str(), "c-")
-		    || ::ink::internal::starts_with(name_override.c_str(), "g-")) {
+		    || ::ink::internal::starts_with(name_override.c_str(), "g-")
+		    || ::ink::internal::starts_with(name_override.c_str(), "s")
+		    || ::ink::internal::starts_with(name_override.c_str(), "b")) {
 			is_knot = false;
-			for (auto itr = name_override.begin() + 2; itr != name_override.end(); ++itr) {
+			for (auto itr = name_override.begin() + (name_override[1] == '-' ? 2 : 1);
+			     itr != name_override.end(); ++itr) {
 				if (*itr > '9' || *itr < '0') {
 					is_knot = true;
 					break;
 				}
 			}
-		} else if (name_override == "s"
-		           || name_override == "b") { // it is not a shared part of a choice
-			is_knot = false;
 		}
 	}
 	handle_container_metadata(*container.rbegin(), meta, is_knot);
@@ -355,7 +355,6 @@ void json_compiler::compile_complex_command(const nlohmann::json& command)
 
 	// Read count
 	else if (get(command, "CNT?", val)) {
-		// TODO: Why is this true again?
 		_emitter->write_path(Command::READ_COUNT, CommandFlag::NO_FLAGS, val, true);
 	}
 

inkcpp_test/Fixes.cpp

@@ -158,3 +158,52 @@ SCENARIO("Casting during redefinition is too strict _ #134", "[fixes]")
 		}
 	}
 }
+
+SCENARIO("Using knot visit count as condition _ #139", "[fixes]")
+{
+	GIVEN("story with conditional choice.")
+	{
+		std::unique_ptr<story> ink{story::from_file(INK_TEST_RESOURCE_DIR "139_conditional_choice.bin")
+		};
+		runner                 thread = ink->new_runner();
+		WHEN("visit knot 'one' an going back to choice")
+		{
+			std::string content = thread->getall();
+			REQUIRE_FALSE(thread->can_continue());
+			REQUIRE(thread->num_choices() == 2);
+			thread->choose(1);
+			content += thread->getall();
+			REQUIRE(content == "Check\nFirst time at one\n");
+			THEN("conditinal choice is displayed")
+			{
+				REQUIRE(thread->num_choices() == 3);
+				CHECK(thread->get_choice(0)->text() == std::string("DEFAULT"));
+				CHECK(thread->get_choice(1)->text() == std::string("Check"));
+				CHECK(thread->get_choice(2)->text() == std::string("Test"));
+
+				WHEN("go to 'one' twice")
+				{
+					thread->choose(1);
+					std::string content = thread->getall();
+					REQUIRE(thread->num_choices() == 3);
+					THEN("get both one strings") { REQUIRE(content == "Check\nBeen here before\n"); }
+				}
+			}
+		}
+		WHEN("loop back to choice")
+		{
+			std::string content = thread->getall();
+			REQUIRE_FALSE(thread->can_continue());
+			REQUIRE(thread->num_choices() == 2);
+			thread->choose(0);
+			content += thread->getall();
+			REQUIRE(content == "DEFAULT\nLoopback");
+			THEN("conditinal choice is not displayed")
+			{
+				REQUIRE(thread->num_choices() == 2);
+				CHECK(thread->get_choice(0)->text() == std::string("DEFAULT"));
+				CHECK(thread->get_choice(1)->text() == std::string("Check"));
+			}
+		}
+	}
+}

inkcpp_test/ink/139_conditional_choice.ink

@@ -0,0 +1,19 @@
+-> test
+
+=== test ===
+
++ DEFAULT
+  Loopback -> test
++ Check
+  -> one
++ {one} Test
+  -> two
+
+=== one ===
+
+{First time at one | Been here before}
+-> test
+
+=== two ===
+
+Knot 2 -> test