Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable dialogue timing options by keeping all flow control in DialogueRunner #95

Closed
loosegrid opened this issue Apr 30, 2021 · 4 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@loosegrid
Copy link
Contributor

Is your feature request related to a problem? Please describe.
(note this is in reference to 2.0 beta 4)
Related to #91 and another issue that I haven't been able to consistently replicate, using automaticallyContinueLines frequently causes errors due to race conditions and/or confusing control flow. It seems like a lot of them stem from the design of YS being mainly focused on user-controlled pacing like text based games and games with many dialogue options. The inclusion of audio clips expands the use cases for YS, but it isn't currently designed to support them as easily. For context, I'm trying to use YS 2.0 for a "walking sim" style game where a narrator is saying lines based on the user's actions and position in the world, but has no control over the pacing and no dialogue options.

Currently, there is no simple way (as far as I can tell) to tell the DialogueRunner to only continue dialogue based on a specific view. For example, the situation in #91 where an audio clip is null but there's still text to be displayed, or a situation where you have multiple active views and a line doesn't need them all. Consider an audio line by an alien character, where the audio line is long, but the text just says "[alien language]".

Likewise, there is no built-in way to have lines stay on screen for a set amount of time. So when set to automatically continue, lines disappear the moment they're finished displaying, making them impossible to read. Exposing a parameter for this would also make it easy to create a player-facing option to slow down text speed as an accessibility option.

Finally, the current configuration makes it extremely difficult to trace errors like the ones mentioned above. In addition to simplifying the control flow, this proposal could possibly enable minor optimizations like no longer needing to constantly build and clear a list of active dialogue views. (An extremely minor optimization most likely, but one that would make me very happy).

Describe the solution you'd like

This is a list of several ideal changes that would address the problem and enable some new features, but the issue could potentially be addressed by only a few of them, or some other solution. The simplest description of what I'd like is just "anything that makes automaticallyContinueLines not break stuff".

  1. Control flow should be kept solely within DialogueRunner and callbacks should only send notifications of a view changing state rather than directly calling functions in DialogueRunner. Callbacks could even be removed entirely and/or just left as events exposed in DialogueViewBase called at the relevant times.
  2. Views should have a state, either instead of or in addition to the current line having a status (sorry to Make LocalisedLine contain line state, not views #8). The state can be checked by DialogueRunner, or it can be updated by an event when it changes, or both, or whatever. The point here is that dialogue views would be much more flexible and decoupled. For example, it would make it easier to create a view that only showed the previous line instead of the current one. This would be good for the common situation of letting messages stay on screen and fade away at their own pace, or to display the previous line as options are displayed, as requested in Run options automatically after RunLine is finished #88.
  3. To allow the previous, DialogueRunner could either check each view's status to see when it's safe to proceed, or view statuses could be kept in a dictionary/list/etc in DialogueRunner and updated by a callback from the view or some other method. This would allow the runner to maintain its own settings for which views should determine timing, block continuation, be ignored, etc.
  4. DialogueRunner could/should only continue lines in LateUpdate, a coroutine using yield return new WaitForEndOfFrame() (cached of course), or any other method which allows all views to get updated before potentially being told to continue/dismiss/etc. Part of DialogueRunner sets CurrentLine to null if audioclip is missing #91 seems to be that some callback leads to CurrentLine being set to null in the middle of a loop, so something like this would hopefully avoid that type of bug on both ends of the exchange. Essentially, a line should never be able to change status twice in one frame.

Describe alternatives you've considered

  • Regarding timing at the end of lines, I manually inserted a delay, but YarnSpinner's configuration as a package makes it incovenient to edit the code. Also as I mentioned, this seems like it should be a built-in feature.
  • I could turn off auto-continue and call StartDialogue manually, but that essentially defeats the point of using YarnSpinner
  • I considered creating a PR for a partial fix, but I'm not sure how to set up git for this type of package and I'd rather get feedback on the preferred solution first.
  • I did think about getting an asset store dialogue system, but that's quitter talk.

Additional context

Some of this would be fairly easy to implement piecemeal, specifically timing the end of lines before auto-continuing could be split off into its own temporary fix.

@desplesda desplesda added the enhancement New feature or request label Jul 9, 2021
@McJones
Copy link
Contributor

McJones commented Aug 18, 2021

Based on some discussions we are thinking the best way forward is to provide two dialogue runners. One runner is the one we all already know and would function basically exactly as is, the second would be purely event driven one. To borrow an analogy from git, a porcelain and plumbing runner.

The current one is the porcelain runner and tries to handle everything for you. The second runner would be a plumbing runner and be far less “out of the box”.
This would be akin to the what you have described above.

In particular the plumbing runner wouldn’t have callbacks or track views or even line states. It would simply fire off line after line, to anything that subscribes to its dialogue firehose.
It wouldn’t care how many views are subscribed or who tells it to advance to the next piece of dialogue, all of that would be up to the various views to manage between themselves, most likely by having one view be the main view that is responsible for that.
Even the existing view calls I think would work without change, they’d just ignore the callback.

It would still craft line types for you, handle localisations, parse markup, interface with variable storage etc because I think these functions are intrinsically important to the dialogue runner.
While there would still be a lot of various concerns to work out I think something along these lines make the most sense.

In the long run the current porcelain runner could even wrap the plumbing runner so that we only maintain a “single” runner.
Although initially they would likely be two completely separate objects.

Thoughts?

@radiatoryang
Copy link
Contributor

+1 to including a simpler minimal runner -- I've been using Yarn Spinner for years and the callback spaghetti still confuses me sometimes

@loosegrid
Copy link
Contributor Author

It's been a few months since I worked with YS so it isn't fresh in my head, but if I'm not missing anything this seems like a good solution. You've got my vote.
My only concern to watch out for would be making sure that putting all these options into the user's hands doesn't cancel out the ease of use benefits of yarnspinner - especially since it would be for less complicated scenarios. However that might end up handled when you wrap them into a single runner, which sounds like a great end goal. Alternatively there could just be a barebones prefab or script using the plumbing runner and that's something even my meager skills could contribute!

@parisba parisba added this to the v2.1 milestone Oct 27, 2021
@desplesda
Copy link
Contributor

The original problem that inspired this issue - DialogueRunner should maintain line state, not line views - has since been resolved in v2.1.0. #130 will remain open, since this is a separate (and useful) issue, but I'm going to close this issue here. Thanks for the useful conversation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants