Migration to Yarnspinner 2.5.0

[Galdormin]

As there are a lot of changes and I'm not familiar with the non-bevy crates, I'll start the migration to the last v2 supported version.

Git diff: YarnSpinnerTool/YarnSpinner@da39c71...v2.5.0

I've looked at the git diff, and this is the files with changes (except doc changes) among the Compiler and Runtime. LanguageServer and tests were ignored.

Compiler

• YarnSpinner.Compiler/Compiler.cs
• YarnSpinner.Compiler/Declaration.cs
• YarnSpinner.Compiler/DeclarationVisitor.cs
• YarnSpinner.Compiler/Project.cs
• YarnSpinner.Compiler/StringTableManager.cs
• YarnSpinner.Compiler/Utility.cs
• (Unsure) YarnSpinner.Compiler/YarnSpinnerLexer.cs => Autogenerated
• (Unsure) YarnSpinner.Compiler/YarnSpinnerLexer.g4

Runtime

• YarnSpinner/Dialogue.cs
• YarnSpinner/Library.cs => NullPointerException, useless in rust
• YarnSpinner/Program.cs
• YarnSpinner/VirtualMachine.cs=> No need refactoring

Test

• YarnSpinner.Tests/ErrorHandlingTests.cs
• YarnSpinner.Tests/ProjectFileTests.cs
• YarnSpinner.Tests/ProjectTests.cs
• YarnSpinner.Tests/TagTests.cs
• YarnSpinner.Tests/TestBase.cs
• YarnSpinner.Tests/TestPlan.cs
• YarnSpinner.Tests/TypeTests.cs => test for validate_unique_inferred_variable (removed in 3.0)

[Galdormin]

@janhohenheim I need help here.

In here, they used 2 fields to store callbacks. I don't think I can do that using a closure in rust, especially because they need a mutable reference of the dialogue.

https://github.com/YarnSpinnerTool/YarnSpinner/blob/838761ad55d8d08be3b46e6f2bea2d017208e445/YarnSpinner.Tests/TestPlan.cs#L205C1-L210C41

https://github.com/YarnSpinnerTool/YarnSpinner/blob/838761ad55d8d08be3b46e6f2bea2d017208e445/YarnSpinner.Tests/TestBase.cs#L216C13-L245C15

I had considered storing all the set variables and allowing a TestPlan function to be called by providing an FnOnce closure. That should work, but I’m not sure if you have any better ideas?

[janhohenheim]

I'll check when the jam is over in 2 days :)

[Galdormin]

@janhohenheim Another question here.

In the Project.cs, they used a C# Matcher to find the files matching the given pattern. However in the test file, they match path/to/Ship.yarn by giving only Ship.yarn. This does not work in Rust with globset, and I didn't find any alternative doing this.

https://github.com/YarnSpinnerTool/YarnSpinner/blob/838761ad55d8d08be3b46e6f2bea2d017208e445/YarnSpinner.Compiler/Project.cs#L185C9-L195C10

One idea to solve this will be to add a few functions, like exclude_filename or exclude_pattern and custom the pattern to match what can work in C#.

• exclude_filename("Ship.yarn") will exclude **/Ship.yarn

Any though on this ?

PS: I will work on the API to use Project in core or bevy_plugin in another PR.

[Galdormin]

I've added serde feature to tests (only test-without-bevy) for now.

I've also added the --no-fail-fast flag to run all tests even in case of failure. I found it easier to track and find regression (especially when tests are failing during implementaion).

[Galdormin]

@janhohenheim In 2023 you created the issue YarnSpinnerTool/YarnSpinner#362 which was fixed.

But did you solve it beforehand in YarnSpinnerRust ? Do I need to port the bug fix ?

https://github.com/YarnSpinnerTool/YarnSpinner-Rust/blob/094c846fbf58194b453bf3de76b3e4e8ed1589ce/crates/runtime/src/virtual_machine.rs#L350C17-L356C24

[janhohenheim]

Okay, I have time now, sorry for the delay :D I'll take a look when I'm home later

[janhohenheim]

In here, they used 2 fields to store callbacks. I don't think I can do that using a closure in rust, especially because they need a mutable reference of the dialogue.

Why wouldn't a Box<dyn FnOnce(&mut DialogueRunner, String) -> String or similar work?

In the Project.cs, they used a C# Matcher to find the files matching the given pattern.

IMO best to use a regular glob there, their syntax looks a bit weird 🤔 We should be fine with just doing **/Ship.yarn instead.

But did you solve it beforehand in YarnSpinnerRust ?

Probably not 🤔 not sure, but I don't think so? Could try if that bug is present in main. In any case, it's not too bad if it's present, as it's fairly esoteric :)

---

Thanks again for this lovely, lovely work you're doing ❤️ It's tremendously generous of you to work on this upgrade!

[Galdormin]

Probably not 🤔 not sure, but I don't think so? Could try if that bug is present in main. In any case, it's not too bad if it's present, as it's fairly esoteric :)

It was not solved, you implemented the test in crashes_on_command_expression_evaluating_whitespace but it was solved by regenerating antlr (I guess 😅)

[Galdormin]

In the end, I didn’t use a callback, it was a bit of a hassle. The function next()of TestPlan takes a mutable reference to the dialogue.

[Galdormin]

@janhohenheim This should be good for review

[janhohenheim]

Heck yeah, on it. I'll mostly check the API and functionality and trust you on the implementation details :)

[janhohenheim]

This looks excellent, thanks! Tested it and it runs well, including web :) I will not go into details about the specific changes since they're ports, but I get most of them and they make sense.
format is the same as format_invariant, right? If so it's definitely useful for production, but also completely fine to leave out for now.
Am I correct in seeing that the Yarn Spinner Project Files are not hooked into Bevy right now? If so, that's completely fine since porting to v3 will presumably touch them again anyways. I assume we can make them a nice and tidy Asset later :)
Are there any big user-facing breaking changes? I don't think so, since you have provided a nice deprecation on add_tags_to_lines. We don't have a changelog rn because of my laziness, but I think it's fine to not enumerate all changes here, since it seems self-evident how to migrate if you look at the signatures.

Again, a thousand thanks! Will merge after I hear back from you :)

[Galdormin]

format is the same as format_invariant, right? If so it's definitely useful for production, but also completely fine to leave out for now.

I'm not sure, format invariant will just convert to string whereas format is more like the classical format {:2} for 2 decimal places and so on.

Am I correct in seeing that the Yarn Spinner Project Files are not hooked into Bevy right now?

Not yet, because I think it needs a bit more discussion. Currently, it loads all yarn files at start into the dialogue manager, but with project we may use the more classical asset.load("project.yarnproject") to load only the files related to the project. I think it's better to open an issue to discuss this.

Are there any big user-facing breaking changes?

None that I'm aware of. Also add_tags_to_lines was mainly called internally, so it may not break anything.

If so, that's completely fine since porting to v3 will presumably touch them again anyways

Just looked at the files we need to port this morning, and it's really something. The oldest commit is from 2021 😢.

[Galdormin]

We don't have a changelog rn because of my laziness, but I think it's fine to not enumerate all changes here, since it seems self-evident how to migrate if you look at the signatures.

From what I remember the changes are :

• Add YarnProject file (not yet in bevy_plugin)
• Extend the standard function library with the functions random, random_range, random_range_float, dice, dec, inc, format_invariant, round, round_places, floor, ceil, decimal, int. Description can be found here https://docs.yarnspinner.dev/write-yarn-scripts/scripting-fundamentals/functions
• The auto line naming uses a hash to have tag like #line:123456
• A few parsing bug of the yarn file (mainly via ANTLR so I cannot enumerate them)

[janhohenheim]

Sheesh that's old!
Yeah agreed, let's look at project files another time in an issue.
I have the suspicion that @andriyDev may even be interested in designing the asset API / mechanisms with us :)