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

Cannot alert description from PhET-iO Archetype. #817

Closed
zepumph opened this issue Jun 6, 2022 · 9 comments
Closed

Cannot alert description from PhET-iO Archetype. #817

zepumph opened this issue Jun 6, 2022 · 9 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jun 6, 2022

Reported originally over in phetsims/friction#277 (comment).

To reproduce:

  • Go to studio in a sim that supports interactive highlights (ratio and proportion/friction/etc)
  • Preferences dialog -> "visual" tab
  • Enable interactive highlights and immediately get an assertion

Assertion failed: must be connected to a display to use UtteranceQueue features

I'll investigate

@zepumph
Copy link
Member Author

zepumph commented Jun 6, 2022

This is the node trying to alert description:

'friction.general.view.navigationBar.preferencesButton.preferencesDialogCapsule.archetype.preferencesPanels.visualPreferencesPanel.interactiveHighlightsEnabledSwitch'

So why is the archetype trying to produce description? How do we shut that off, but mostly, why is it listening to real-world items. I'll try to shut that off.

@zepumph
Copy link
Member Author

zepumph commented Jun 6, 2022

I made some progress here with a commit here, in it we opt out of description and voicing responses if we are a PhET-iO archetype, but this isn't complete, and you can see that with the same assertion when enabling voicing from the preferences dialog. This is because the item trying to emit voicing is not an instrumented Node, and thus this.phetioIsArchetype === false. So I think we may need to design this a bit more, perhaps another assertion, or say that only instrumented dynamic element Nodes can produce Description/Voicing responses. I'm not really sure if @jessegreenberg or @samreid would be a more helpful consultant on this issue. I think I'll start with @samreid since it is so much about PhET-iO dynamic elements. Let's talk!

@samreid
Copy link
Member

samreid commented Jun 6, 2022

I don't think I know enough about connectedDisplays or why there is no connected display to move forward on this. Want to schedule a meeting?

@samreid samreid removed their assignment Jun 6, 2022
@zepumph zepumph changed the title Cannot turn on Interactive Highlights in PhET-iO Studio Cannot alert description from PhET-iO Archetype. Jun 6, 2022
@samreid
Copy link
Member

samreid commented Jun 6, 2022

@zepumph and I worked out this patch which determines dynamically whether a node descendant has a phetioIsArchetype parent:

    function visit( node: Node ): boolean {
      const parents = node.parents;
      for ( let i = 0; i < parents.length; i++ ) {
        if ( isPhetioArchetypeDynamically( parents[ i ] ) ) {
          return true;
        }
      }
      return false;
    }

    function isPhetioArchetypeDynamically( node: Node ): boolean {
      return node.phetioIsArchetype || visit( node );
    }

@zepumph
Copy link
Member Author

zepumph commented Jun 7, 2022

This worked in practice, but I think I can make it one function

  isInsidePhetioArchetypeImplementation( node: Node ): boolean {
    const parents = node.parents;
    for ( let i = 0; i < parents.length; i++ ) {
      if ( this.isInsidePhetioArchetype( parents[ i ] ) ) {
        return true;
      }
    }
    return false;
  }

  isInsidePhetioArchetype( node: Node = ( this as unknown as Node ) ): boolean {
    return node.phetioIsArchetype || this.isInsidePhetioArchetypeImplementation( node );
  }

zepumph added a commit to phetsims/scenery that referenced this issue Jun 7, 2022
@zepumph
Copy link
Member Author

zepumph commented Jun 7, 2022

Ok. I got it to one function. I also made it so that it will only check in phet-io brand.

With this patch I saw that after playing with the sim for 30 seconds or so, the function had only been called 250 times. Even so I think there is a nice optimization we can do. If you encounter any PhET-iO instrumented node going up the tree, then you are done, if it is instrumented and not an archetype, then you know that your uninstrumented Node child is not an archetype also. I'll do that now.

zepumph added a commit to phetsims/scenery that referenced this issue Jun 7, 2022
zepumph added a commit to phetsims/scenery that referenced this issue Jun 7, 2022
@zepumph
Copy link
Member Author

zepumph commented Jun 7, 2022

That is working very well. I also added this same recursion to Voicing as well. The issue that I have is for usages of Voicing.alertUtterance. Here we don't have a Node, and so we don't have access to this function. I will want to discuss this with @jessegreenberg at our meeting on Friday.

@jessegreenberg
Copy link
Contributor

I took a look at the above commits but I don't understand the implications or expected behaviors. The previous comment referenced review at an upcoming meeting, and I am certainly happy to understand more but I also trust PhET-iO team here! Reassigning to @zepumph.

@jessegreenberg jessegreenberg removed their assignment Aug 29, 2022
@zepumph
Copy link
Member Author

zepumph commented Oct 7, 2022

@jessegreenberg and I discussed this today, we understand and can't think of any other loose ends. Closing

@zepumph zepumph closed this as completed Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants