phetsims / scenery

Scenery is an HTML5 scene graph.
MIT License
53 stars 12 forks source link

We need an easy way for a Node to determine whether it is visible to the user #1074

Open jbphet opened 4 years ago

jbphet commented 4 years ago

While implementing the sound design for Faraday's Law, I need to add a sound for the case where the needle on the voltmeter "pegs" at a max or min value. It seemed like the most logical place for this sound generation to occur was in the node that represented the gauge, which is VoltmeterGauge.js. However, the sound should only be generated when the gauge is visible, and it is several levels down in the scene graph from the place where the overall visibility of the voltmeter is controlled. I asked on Slack what the best way for a node to determine whether it was visible to the user, and a fairly long dialog ensued. For reference, here is the dialog:

``` John Blanco 1:42 PM What's the easiest way for a Scenery node to determine if it is visible when it has several parent nodes? Jesse Greenberg 1:45 PM Can you assume the Node doesnt use DAG? Maybe node.getUniqueTrail().isVisible() Jonathan Olson 1:46 PM Presumably you mean it has at least one visible instance on the main display? John Blanco 1:47 PM Yes, that's what I mean. I basically want to add code in the node that says, "If I'm visible, generate sound, otherwise don't". Jonathan Olson 1:49 PM _.some( node.getTrailsTo( phet.joist.display.rootNode ), trail => trail.isVisible() ) John Blanco 1:50 PM I'm glad I asked - I would never have come up with that. I could see this coming up again in relation to sound generation. Should we consider a method to make this a bit more convenient? Jonathan Olson 1:52 PM I could imagine display.hasVisibleTrailTo( node ) factoring that out Chris Malley 1:52 PM How about node.isDisplayed? 1:53 display.hasVisibleTrailTo( node ) seems backwards. John Blanco 1:53 PM The recommended code works great, by the way. Jonathan Olson 1:53 PM You would recommend node.isDisplayed( phet.joist.display ) then? Chris Malley 1:53 PM With phet.joist.display as the default? (edited) Jonathan Olson 1:53 PM And "isDisplayed" ... it may have no opacity, might be not within the bounds, might be covered up with something 1:54 I really don't want to be hardcoding phet.joist.display in Scenery Chris Malley 1:54 PM Trying to avoid having to know about phet.joist.display. I have a Node and I want to know if it's being displayed. Jonathan Olson 1:54 PM I could imagine a method to see if it's shown by ANY display 1:54 (that's why I asked about the requirements above) Chris Malley 1:55 PM How often do we have > 1 display in a sim? Jonathan Olson 1:55 PM It's a non-trivial number of sims Sam Reid 1:55 PM What about a method implementation like: isRecursivelyVisible(){ return this.visible && _.some(parent,isRecursivelyVisible); } Chris Malley 1:56 PM So you recommend phet.joist.display.hasVisibleTrailTo( node ) ? 1:56 ... for the typical case. (edited) Jonathan Olson 1:56 PM I'm open to other suggestions Chris Malley 1:57 PM I'll never remember it and will be asking you again. It's still better than the above _.some(...) magic. Jonathan Olson 1:57 PM Understood, but what would be better than that? 1:58 And isRecursivelyVisible() --- what would it do when there are no parents? Sam Reid 1:58 PM Then it would return true if the node is visible:true Jonathan Olson 1:58 PM It's possible to write a "is there a visible trail to ANY display", but that doesn't seem like what is desired Sam Reid 1:58 PM Oh I see, we need to check if it is in a display and visible Chris Malley 1:58 PM Well... First it depends on whether we want to know if a Node is displayed (a method on Node), or whether a Display is displaying a Node (a method on Display). Jonathan Olson 1:58 PM So... new Node().isRecursivelyVisible() would be true? 1:59 or a node disconnected from the screenview would have true? Chris Malley 1:59 PM Personally, I never think of Display, and I have little knowledge of what it is/does. Sam Reid 1:59 PM John B, if a Node is visible but not attached to the scene graph, should it make sound? John Blanco 1:59 PM What about node.isVisibleOnDisplay( display )? Then a developer would see it in Node.js, search for usages, and figure out how to use in the most general case. 2:00 Sam R - In the case you're describing, I think the node would not be visible to the user, so no, it should not make sound. Sam Reid 2:00 PM It’s possible to write a “is there a visible trail to ANY display”, but that doesn’t seem like what is desired That seems like what is desired, since a Node should make sound if it is visible in any display. Jonathan Olson 2:00 PM I'm also fine with node.isVisibleOnDisplay( display ) 2:01 That seems like what is desired, since a Node should make sound if it is visible in any display. I'm not convinced of that John Blanco 2:02 PM How about that and node.isVisibleOnAnyDisplay()? (edited) Sam Reid 2:02 PM What if we had isVisibleOnDisplay( display = null) where null check against any display? Jonathan Olson 2:02 PM e.g. NodeTexture -- it uses a Display to render a changeable Node into a three.js texture, so that it can be displayed. It might NOT be visible in the sim at any given moment Sam Reid 2:02 PM Ah I see John Blanco 2:04 PM Shall I log an issue and paste in this dialog? Jonathan Olson 2:05 PM Sure. I'm ok with a null check, but I can't imagine ever personally using it ```

Marking for dev meeting so we can finalize the solution and determine the priority.

samreid commented 3 years ago

Two of the proposals above are:

phet.joist.sim.display.hasVisibleTrailTo( node );
node.isVisibleOnDisplay( phet.joist.sim.display );

Is the latter more developer friendly since it is documented in Node? I don't have a strong preference between these options.

jessegreenberg commented 3 years ago

Regarding the two proposals above, @jonathanolson mentioned that the first makes more sense because this should not be information on the Node, it should be the responsibility of the Display.

The team was convinced, and we are going to try phet.joist.sim.display.hasVisibleTrailTo( node ) for now.

jessegreenberg commented 3 years ago

@jessegreenberg will work on this.