Luca3317 / TMPEffects

Easily animate Unity text and apply other effects with custom tags
https://luca3317.dev/tmpeffects
MIT License
248 stars 11 forks source link

API Usability Bug: TMPWriter Events require a TMPWriter parameter in listening functions #24

Closed AlejandorLazaro closed 1 month ago

AlejandorLazaro commented 1 month ago

Describe the bug Compared to Yarn Spinner UnityEvents which don't have extra parameters (see following) or a string parameter, TMPWriter events have TMPWriter as part of the event definition, requiring callback functions to have a TMPWriter parameter, even if they don't need it and have their own separate internal logic.

Sample code with the error:

    public void Start() {
        dialogueRunner = FindObjectOfType<Yarn.Unity.DialogueRunner>();
        tmpLineWriter = GameObject.Find("Dialogue System/Canvas/Line View/Text").GetComponent<TMPWriter>();

        // Set event listeners for the DialogueRunner for key conversation
        // changes
        dialogueRunner.onDialogueComplete.AddListener(EndConversation);  // UnityEvent

        // Set event listeners for the TMPWriter for key conversation changes
        tmpLineWriter.OnPauseStartedWriter.AddListener(OnFinishWriter);  // UnityEvent<TMPWriter>
    }

    private void EndConversation()
    {
        // func body
    }

    private void OnFinish(/*TMPWriter tmpWriter*/)  // Complaining because it doesn't have a TMPWriter parameter; works with the commented out code
    {
        // func body
    }

Results in the following error: Assets/Scripts/RobotYarnInteractable.cs(82,56): error CS1503: Argument 1: cannot convert from 'method group' to 'UnityAction<TMPWriter>'

Yarn Spinner Events ```c# /// /// A Unity event that is called when the dialogue starts running. /// public UnityEvent onDialogueStart; public class StringUnityEvent : UnityEvent { } /// /// A Unity event that is called when a node is complete. /// /// /// This event receives as a parameter the name of the node that /// just finished running. /// /// public StringUnityEvent onNodeComplete; ```
TMPWriter Events ```c# /// /// Raised when the TMPWriter is done writing the current text. /// public UnityEvent OnFinishWriter; /// /// Raised when the current (section of) text is skipped. /// public UnityEvent OnSkipWriter; ```

To Reproduce Steps to reproduce the behavior:

  1. Have a Unity scene with a TMPro object and TMPWriter component attached
  2. Have a script which adds event listeners to a TMPWriter event with a no-parameter callback function
  3. See the compilation fail when Unity resolves the files.

Workaround Adding a TMPWriter tmpWriter to the callback function's parameters removes the compile errors.

Expected behavior TMPWriter events should be able to be listened/subscribed to without needing a passed in TMPWriter object.

Desktop (please complete the following information):

Luca3317 commented 1 month ago

I added the TMPWriter argument so classes that listen to events on multiple TMPWriters with the same methods can easily differentiate which TMPWriter raised the event (analogous to the object sender parameter in standard C# events).

Aside from the ease of being able to subscribe parameterless functions, is there some advantage to this Im missing?

Edit: I see that the TMPWriter parameter is missing from the docs though, will add those (if I keep them)

AlejandorLazaro commented 1 month ago

Ah, gotcha! That was a use case I was worried about but hadn’t experimented with, but the concept makes sense.

If/when you get the chance to make some docs on TMPEffects + external dialogue manager packages, mentioning that design decision and having a little example should be enough for people to understand why they should be passing and validating the TMPWriter instance that triggered the call is the right one.

In my naivety (still newish to C#), I did have the impression I could listen to a specific TMPWriter object in a scene, and at that point I wouldn’t even need to validate the triggering TMPWriter. I think it’d be easy enough to test though, so I’ll see if my impression is off base.

Luca3317 commented 1 month ago

In my naivety (still newish to C#), I did have the impression I could listen to a specific TMPWriter object in a scene, and at that point I wouldn’t even need to validate the triggering TMPWriter. I think it’d be easy enough to test though, so I’ll see if my impression is off base.

You do, but there might be cases where you want to listen to multiple TMPWriters and do different stuff depending on which one raised it. Niche cases for sure but I like the flexibility it gives you.

It does still seem neater to have a parameterless versions to listen to especially for quick hooks in the inspector. Id add both versions for each, but the inspector event foldout is getting a bit thick... I'll think a bit more about which I prefer

AlejandorLazaro commented 1 month ago

While the parameter-less version sounds nice for my first-time approach, I'd say if the standard use case of TMPEffects is to work with scenes that may have many TMPWriters/Animators, then passing in the TMPWriter for later validation is vital—and I think is worth forcing even in a one-TMPWriter scenario, just to be safe and keep a consistent API.

(Example: a minimum of ~3 when Yarn Spinner is being used with a "Text", "Last Line", and "Option Text" TMP objects with any number of TMPW/A's on em).

Also, keeping the UI clean is a noble consideration.

Luca3317 commented 1 month ago

I just thought about this again, going back and forth on it but just remembered you can totally hook up parameterless methods in the inspector either way 🤦🏽‍♂️ That was the main argument against keeping the Writer parameter, so yeah I agree, lets keep them