Skip to content

Fix knot tracking#159

Open
willvale wants to merge 7 commits into
JBenda:masterfrom
willvale:fix-knot-tracking-2
Open

Fix knot tracking#159
willvale wants to merge 7 commits into
JBenda:masterfrom
willvale:fix-knot-tracking-2

Conversation

@willvale

@willvale willvale commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Re-opening so I can test the workflow, but probably not ready yet.

Locally, everything passes (ink-proof, inkcpp-test) except an obscure failure in one of the migration tests while initialising globals (before the migration or anything interesting happens) which I haven't figured out yet.

willvale added 6 commits June 29, 2026 21:44
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.
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.
Can't just run ClangFormat in VS as that makes further changes unrelated to the code changes.
@willvale

willvale commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

OK, this is now in sync with what I have locally. Will keep looking.

…rrent 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.
@willvale

willvale commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@JBenda I would have a look at this in detail - it's possible that by tracking the current knot across call/return, I might be breaking assumptions in inkcpp about what the current knot can be.

The workaround I added above seems to be fine for inkcpp-test and ink-proof, but I'm not sure if it's missing something in migration because I don't know too much about that. See what you think.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Ink Proof Results

These results are obtained by running the Ink-Proof Testing Suite on the compiled binaries in this pull request.

System Results
Linux x64 130/130 passed
MacOSX-ARM DISABLED
MacOSX DISABLED
Windows x64 130/130 passed

@JBenda

JBenda commented Jul 4, 2026

Copy link
Copy Markdown
Owner

First, thanks for the PR. I took a first look, and it looks good. I should document assumptions more, but to be real, I forgot about knots and functions when writing the migration logic.

I will sleep a night and look at the code again, but I will probably merge it as it is. The local variable init for migration is not the finest piece of code, but it should work good enough for the start.

PS: Nice spot that we can move the preserve_turns out of the function, one magic bool flag less.

@willvale

willvale commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

NB: The reason I wrote this is I'm starting to do some more complicated things with inkcpp in my game project to support dropping and resuming dialogue.

So I can interrupt a conversation in the middle, play a "where did you go" line on a secondary runner and then eat dialogue until I reach a "resume" tag or leave a resumable knot. And then continue normally. This relies on line tags (which were working) and knot tags (which didn't work for tunnels, which I use a lot).

I still need to do the same kind of thing for resuming backwards (e.g. if there's dialogue that's important, we need to go back to where it started) so it's possible that will generate more PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants