inkle / ink-unity-integration

Unity integration for the open source ink narrative scripting language.
http://www.inklestudios.com/ink
Other
571 stars 99 forks source link

- #57

Closed ghost closed 6 years ago

tomkail commented 6 years ago

Hey! Good spot - we basically never remove observers. The right place to discuss this is in the Ink github page (this is for the unity package) - https://github.com/inkle/ink - if you'd like to submit a pull request we'll take it in!

On Thu, Mar 29, 2018 at 10:41 AM, stqn1 notifications@github.com wrote:

Hello. The RemoveVariableObserver function has a couple bugs:

  • if you call it without providing a variable name, it crashes because you are not allowed to change a dictionary while iterating on it.
  • if you call it providing a variable name, it works, but the dictionary entry stays even when the last observer was removed, so the observer value is null, and it causes another crash later when the variable is modified.

Here is a proposed fix (sorry for not making a pull request, and also I didn't try it):

    public void RemoveVariableObserver(VariableObserver observer, string specificVariableName = null)
    {
        if (_variableObservers == null)
            return;

        // Remove observer for this specific variable
        if (specificVariableName != null) {
            if (_variableObservers.ContainsKey (specificVariableName)) {
                _variableObservers [specificVariableName] -= observer;
                if (_variableObservers [specificVariableName] == null) {
                    _variableObservers.Remove (specificVariableName);
                }
            }
        }

        // Remove observer for all variables
        else {
            foreach (var varName in _variableObservers.Keys.ToList()) {
                _variableObservers [varName] -= observer;
                if (_variableObservers [varName] == null) {
                    _variableObservers.Remove (varName);
                }
            }
        }
    }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/inkle/ink-unity-integration/issues/57, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMd0AcuOfzFELfv68-itbVgwnyY54v2ks5tjKxJgaJpZM4TACrd .