Correctly parse tags related to each line

[MrHands]

My major goal with this pull request was to support retrieving the correct tags per line, and I've succeeded with that. Unfortunately, I also broke several existing tests in the process and haven't been able to get them working yet. I don’t think it’s very nice of me to leave your project in a worse state than I found it, so I’m providing you with as much documentation as possible about where I suspect the problems lie. I hope we can work together to integrate my changes into your main branch.

Major issues

While all tests passed for adding tags to lines and parsing global tags, my changes caused 12 failed assertions and three failed test cases in the existing unit and integration tests. This is because my changes require looking ahead to the following line to see if we have glue or choices that could affect the current line. But this spawns several edge cases I tried to handle as they popped up from testing, and the logic is now very complicated. I suspect there is a much cleaner solution, but I just can’t see it yet. Here are the major issues that came up in my testing.

Glue is not parsed correctly across multiple lines

Specifically, the first glue command is handled correctly, but subsequent commands are not processed as part of the same line. This results in breakages like these:

Scenario: Observer
      Given: a story which changes variables
       When: Run without observers

C:\Projects\inkcpp\inkcpp_test\Observer.cpp(25): FAILED:
  CHECK( thread->getall() == "hello line 1 1 hello line 2 5 test line 3 5\n" )
with expansion:
  "hello line 1 1 hello line 2 5
  test line 3 5
  "
  ==
  "hello line 1 1 hello line 2 5 test line 3 5
  "

I suspect this is caused by my changes to read ahead in the step() method. It has proven very tricky to read ahead enough to check if the following command is part of the current line while not accidentally calling functions on the next line.

External functions assert due to command buffer depletion

Again, I suspect this is caused by my changes to look ahead in the command buffer. Here’s what the failure looks like:

C:\Projects\inkcpp\inkcpp_test\ExternalFunctionsExecuteProperly.cpp(26): FAILED:
  {Unknown expression after the reported line}
due to unexpected exception with message:
  fallback function but no function call before?

This assertion triggers on line 1129 in my changes, but I suspect the problem occurred much earlier. I suspect that the look-ahead already executed the function, and it cannot be found again on line 1228:

} else if (_output.saved() && _output.ends_with(value_type::newline, _output.save_offset()) && ! fn->lookaheadSafe()) {

Changelog

• choice class: Tags are stored as snap_tag* held by the runner_impl instead of const char*.
• runner interface: Added virtual num_global_tags() and get_global_tag() methods.
• runner interface: Added line_type for std::string or FString depending on compile settings.
• runner interface: Simplified getline() method to return line_type.
• basic_stream: Reimplemented entries_since_marker() as find_first_of() and find_last_of() methods.
• basic_stream: Removed saved_ends_with() method in favor of an offset parameter to the ends_with() method.
• basic_stream: Use size_t instead of int for offsets into the data.
• basic_stream: Protect against buffer underflow in the discard() method.
• basic_stream: Added npos constexpr for invalid positions in the buffer.
• runner_impl: Added add_tag() and clear_tags() methods to manage tag info.
• runner_impl: advance_line() clears line and choice tags while keeping the global ones.
• runner_impl: line_step() reads steps from the execution buffer until the next new line.
• runner_impl: line_step() copies global tags to the first line.
• runner_impl: line_step() continues stepping execution until it's satisfied it doesn't affect the current line.
• runner_impl: Added set_debug_enabled() to optionally write command info to a debug stream as the instance steps through the execution buffer.
• runner_impl: step() writes debug info to the debug stream, if available.
• runner_impl: Streamlined getline() implementation to work with either std::string or FString.
• runner_impl: Streamlined getall() implementation to call getline() multiple times, reducing the need to test this method separately.
• runner_impl: Simplified detect_change() to check for extending past the newline only.
• command: Added std::ostream operator for streaming Command and CommandFlag enum values as strings.
• snapshot_impl: Added text() method to retrieve stored string directly.
• snapshot_impl: Store pointers to tags start and end instead of text pointers.

Unit test changes

• Replaced many REQUIRE() calls with CHECK() to ensure test execution isn’t halted immediately.
• Tags.cpp: Added unit tests for adding and removing tags to a runner.
• Tags.cpp: Made loaded story global to ensure that WHEN() and THEN() tests can be run in sequence correctly.
• NoEarlyTags.cpp: Added checks for specific tags after each line.
• NewLines.cpp: Cleaned up tests.
• TagsStory.ink: Expanded tests for tags in different places.
• LinesStory.ink: Added a condensed test case for ignoring functions when applying glue.

[JBenda]

Ah, they updated the Ubuntu runner. We should update some dependencies:

Thank you for the detailed description of your changes, and the occurring problems, that will save me a lot of time reviewing the changes.

I will now think a bit loud:

About external functions a glue, this is a known limitation, and can be handled lookaheadSafe in which case the function will just be called multiple times. So, in theory, your idea should work.

• [ ] .github/workflows/build.yml clang-format-14 with clang-format-18.
• [ ] check external function call rollback runner_impl
• [ ] why different behavior in further look ahead?
• [ ] check UE plugin new changes
• [ ] check functionality lookahead function binding setting

[JBenda]

A sorry, it seems you are figuring out the auto formatter, it only tests as part of the pipeline, it does not change the code and re commit it. So one way is to execute the command locally or you wait and I will do it later one. It is not necessary for the unit tests to run.

[MrHands]

Yeah, I'm not figuring it out on my own, sorry. :(

You were right that the command needed to change to 18 to work, but I haven't figured out how to run it locally yet. It's also been a long day, and I will try again tomorrow.

[JBenda]

Locally I execute:

$ git clang-format --extensions c,cpp,h,hpp --style file master

If you run windows, you can install LLVM (including clang-format) with

winget install BrechtSanders.WinLibs.POSIX.UCRT.LLVM

It is currently version 19.1.1, but I did not notice any breaking changes.

[JBenda]

Work in progress, If the line already ends with a newline (what it does if we are looking for glue and have a code piece in between) We will immediately stop, therefore the function line_step should ignore a newline already present at the beginning.

My first attempt to fix it (which seems to work): I will follow up with a proper commit once I have figured out the external function call.

It seems to be something funny because it has a fallback function, but it's not lookaheadSave So it is not executed because of it should not, but then the fallback tries to trigger, which is not the intended behavior. The lookaheadsafe is not properly gated, a missing null check ...

runner_impl.c:725

size_t   already_had_newline = _output.find_last_of(value_type::newline);
size_t last_newline        = basic_stream::npos;
while (_ptr != nullptr && (already_had_newline == last_newline || last_newline == basic_stream::npos)) {
	step(debug_stream);
	last_newline = _output.find_last_of(value_type::newline);
}

[MrHands]

Thank you for this surgical suggestion! This indeed fixes several failing assertions caused by my changes. The remaining problem is that external functions aren't handled properly.

I've also found that Visual Studio has built-in support for clang-format, so I've pressed Ctrl+K, Ctrl+D on all my changes to ensure they comply with your preferred code style.

[JBenda]

I'm currently working on it in the branch scoped_tags I needed to reverse the changes in the step_line() function since the new mechanism was incompatible with some assumptions. Things to do before putting it here:

• [x] add tests for knot tags (correct assignment)
• [x] fix ink proof test I100 (choice tags from first choice are assigned to line if no text is above the choice)
• [x] replace static start variable in line_step() to
• [x] replace _jumped variable in runner_impl with a more robust concept problem: detection if we entered a new knot/stitch for tracking Knot-tags
• [x] reinsert debug output in step function
• [x] add knot/global tag functions to C and python interface
• [x] add tests for C and python interface
• [x] add Unreal blueprint bindings?

[JBenda]

Works under windows, fails differently under mac and Linux ... I will look into this soon, if you have an idea, feel free to check

[MrHands]

Thanks so much for iterating on my PR! Although I only have access to Windows machines, I'm familiar with the peculiarities of Linux and Clang. I'll see what I can do. 😊

[JBenda]

This was stupid, it seems MSVC and clang have different execution order policies The problematic line was x.insert(i) = x[i+d] with MSVC it was first evaluated x[i+d] and then x.insert(i), clang with Unix has done it the opposite, which resulted in copying the wrong element.

Would you mind checking if this PR still looks correct ^^ If you give your ok I would bump the version and bump the version to 0.1.8

(When I finished a basic UE binding [and tested it])

[MrHands]

I'm happy to confirm your changes work perfectly for my game!

LGTM, let's ship it!! 🫡

[JBenda]

I tested it today a bit and noticed that the knot tracking fails if you jump to a stitch of another knot (you keep your current knot tags) I think instead of knot tags we should track stitch/knot tags (whatever we are currently in) This is after a short feedback round the most intuitive functionality and is easier to implement because knot tags (without stitches) introduce many corner cases, like jump to the stitch, but missing the knot tags from above etc.

[JBenda]

Implemented the solution, added a get_current_knot() function which returns the ink::hash_string of the current knot/stitch

TODO:

• [x] implement get_current_knot() for python and C
• [x] change documentation to underline that get_knot... actually means knots and stitches