need a generalized model element creator node

[jbphet]

There is a behavioral pattern that is used in a number of PhET's simulations where a Scenery node exists somewhere in the view - often in a carousel or tool box - and can be clicked and dragged in order to create a 'real' instance of the item that the node represents. @pixelzoom recently needed to create such behavior in function-builder, and used ShapeCreatorNode in area-builder as a starting point. He had some suggestions for improvements to ShapeCreatorNode, and while implementing these suggestions, I searched through the codebase and found roughly nine additional places where similar code and functionality appeared. Here is a list by repo and source file:

• balancing-act ModelElementCreatorNode.js
• bending-light various places
• capacitor-lab VoltmeterToolBoxPanel.js
• capacitor-lab-basics VoltmeterToolBoxPanel.js (this should probably be consolidated with the capacitor-lab version)
• charges-and-fields ChargesAndFieldsToolboxPanel.js
• energy-forms-and-changes ThermometerToolBoxNode.js
• expression-exchange CoinCreatorNode.js
• least-squares-regression DataPointCreatorNode.js
• making-tens PaperNumberCreatorNode.js

We should consider creating a generalized version of this (e.g. ModelElementCreatorNode) that can, at the least, be used when we need to do such things in the future and, at the most, can be used to replace most of the duplicated code.

This was discussed in the 1/7/2016 developer meeting.

[jbphet]

I'm assigning this to myself to start with and will implement the suggestions that @pixelzoom made in phetsims/area-builder#83 in each of the sims listed above where it makes sense to do so.

[pixelzoom]

FYI, my version of this is function-builder.FunctionCreatorNode. Some of the responsibilities of ShapeCreatorNode were moved elsewhere. From the JSdoc, FunctionCreatorNode has the following responsibilities:

 * - displays a function's icon
 * - creates {AbstractFunction} function instances and manages their initial drag sequence
 * - limits the number of instances created to some (optional) maximum
 * - hides the function's icon and ceases creation when the maximum number of instances is reached
 * - monitors function instances to determine when they have been disposed of

This will likely be generalized further when I create function-builder.CardCreatorNode.

[jbphet]

I implemented the suggested improvements from phetsims/area-builder#83 in all of the repos listed in comment #214 (comment) except for bending-light, which is doing things a bit differently. Assigning to @pixelzoom to follow up after doing more generalization for function-builder. We can pick up the discussion then to see if we feel like there is a reasonable way to generalize this functionality.

[pixelzoom]

Unlikely that I'll get to this until we're in the end game for Function Builder. So sometime in August or September 2016.

[samreid]

ShapeCreatorNode is searching up the tree for the ScreenNode. Is this because that is the view coordinate frame associated with the model view controller?

[pixelzoom]

ShapeCreatorNode searches for the parent ScreenView. And yes, the parent ScreenView's local coordinate frame is assumed to be equivalent to the model coordinate frame (there is no model-view transform). See line 114 & 116.

[pixelzoom]

Put a TODO in FunctionCreatorNode to remind me to address this later in the function-builder development process. Unassigning until then.

[samreid]

Here are my notes from when I was investigating this issue recently:

"Add a drag handler for items in a toolbox/carousel/bucket" #186

In the issue description, I identified simulations with toolboxes and the different kinds of dragging behavior we would need to support.

[samreid]

For Bending Light, I developed ToolIconListener.js which operates by transmitting events from the toolbox icon to the play area instance. This makes it very easy to reuse the MovableDragHandler, including its constrained drag bounds and making sure the drag behavior when dragging from the toolbox matches the drag behavior when dragging from the play area. Here is sample usage from IntroView.js:

    // Add an input listener to the toolbox icon for the protractor, which forwards events to the MovableDragHander
    // for the node in the play area
    protractorNodeIcon.addInputListener( new ToolIconListener( [ protractorNodeListener ], function( event ) {
      // Show the protractor in the play area and hide the icon
      introView.showProtractorProperty.value = true;

      // Center the protractor on the pointer
      protractorLocationProperty.value = protractorNode.globalToParentPoint( event.pointer.point );
    } ) );

ToolIconListener also supports nodes with multiple components (such as probes with movable body and sensor).

[samreid]

ToolIconListener currently only supports pre-existing model items, but we could modify it so it dynamically creates items when the toolbox item is dragged. The main advantage of the ToolIconListener strategy is that the behavior of the drag listener is identical whether the object is dragged from the toolbox or from the play area--for instance if (a) something is supposed to happen when the item is dropped in the play area or (b) keeping the node constrained to the play area bounds these would happen automatically because the same input listener is used.

Looking into how this would be done for ModelElementCreatorNode.js: currently ModelElementCreatorNode.js requires the following API to work: The node must provide addElementToModel for creating the model element. During drag, the model element position is set via thisNode.modelElement.position. On drag end, thisNode.modelElement.release(); is called. How could we change this to support keeping an object in-bounds during dragging? We would have to move the functionality in MovableDragHandler to the model elemen--perhaps keeping track of its desired position, available bounds and actual position. Then the ModelElementCreatorNode would set the desired position and the model element would use the available bounds to position the model element.

Comparing to FunctionCreatorNode.js: it looks like Function Builder does not yet support keeping objects within bounds when dragging them, so an approach like the one mentioned above may need to be used.

To summarize the differences between ToolIconListener and ModelElementCreatorNode/FunctionCreatorNode: ToolIconListener works by reusing logic in an input listener and MECN/FCN work by reusing logic in the model

There is also the matter of PhET-iO instrumentation. Since ToolIconListener uses MovableDragHandler, it is automatically instrumented for streaming data delivery. If we proceed with a MECN/FCN generalization, it would need to have the same feature.

Finally, a reminder that ToolIconListener currently only supports pre-existing model elements and if we continue with it then it should be modified to (optionally) dynamically create new instances.

Finally Part II: the other feature ToolIconListener supports that I'm not sure how to support in MECN/FCN is dragging of composite objects and keeping them all in bounds. Perhaps we would need a composite model element that forwards calls to its children, but I'm not sure exactly how this would work.

One last thought: If we move to a pattern like MECN/FCN where the logic for dragging something in-bounds is handled in the model (after obtaining the available bounds from the view), then I don't see the need for MovableDragHandler--that logic for keeping a node in bounds would be generalized in model code and SimpleDragHandler could use that model code directly.

Thinking through the possibility of factoring out this logic to the model--I often check for an intersection between the global bounds of a node and the global bounds of a toolbox panel to decide whether the user is returning an object to the toolbox or keeping it in the play area. This seems straightforward for TIL and we would need some way of doing this in MECN/FCN such as keeping the bounds of the toolbox in model coordinates for bounds tests on release--note: these bounds would have to be synchronized as some simulations have "floating control panels" whose positions adjust based on the aspect ratio of the screen. Also, in Bending Light, when a node is dropped on top of (or behind) another control panel, it is supposed to "pop out". This computation is natural to do in the view. How could we implement this in MECN/FCN? When the model element release function is called, Node/Node bounds comparisons could be done in the view and more messages could be sent back to the model.

TLDR: we need to decide whether to handle dragging features in the model or view, and make sure our solution provides the following features: keeping a node within visible bounds, dropping it back into a (possibly movable) toolbox, popping it out when dropped in front of another (possibly movable) control panel, sending an input listener data stream to PhET-iO, and the ability to create new model elements or reuse existing model elements.

[samreid]

Tagging @jonathanolson and marking for developer-meeting in case we cannot conclude the discussion before then.

[jbphet]

At the dev meeting on 1/12/2016 we all agreed that this is a good idea, but no one has enough time to give it the attention required to do it well. I will try to give it some thought as I work on Expression Exchange, @pixelzoom will do the same while working on Function Builder, and @samreid as he works on CCK.

[pixelzoom]

Function Builder is ultimately going to use a different pattern, and will not be creating model elements dynamically. And since I didn't create any of the "duplicate" code identified in #214 (comment), I have no plans to work on generalizing.

[pixelzoom]

In 1/21/16 dev meeting, @jonathanolson identified this bit of code as particularly "smelly" (my adjective). This is shared by most (all?) of the above "duplicates".

    if ( !parentScreenView ) {
          // Move up the scene graph until the parent screen is found.
          var testNode = thisNode;
          while ( testNode !== null ) {
            if ( testNode instanceof ScreenView ) {
              parentScreenView = testNode;
              break;
            }
            testNode = testNode.parents[ 0 ]; // Move up the scene graph by one level
          }
          assert && assert( parentScreenView, 'unable to find parent screen view' );
        }

A couple of things:

(1) It would be better to simply pass in the appropriate ancestor node (which shouldn't have to be a ScreenView), rather than searching up the scenegraph.

(2) The parent ScreenView is used to transform pointer location from view to model coordinate frame. The assumption (undocumented in most implementations) is that the ScreenView's local coordinate frame is equivalent to the model coordinate frame. In general, that is unlikely to be the case. A more general implementation should support a non-unity model-view transform. One way to do that would be to pass in a function (constructor param? option?) that does the transformation, something like: {function({Event}): {Vector2) } viewToModelVector2

[samreid]

@jonathanolson suggested factoring out that code if it will be widely used.

@ariel-phet suggested a developer meeting topic to decide whether nodes should be "swipe to snag" by default or not.

[pixelzoom]

Labeling with meeting:developer, since I'm not sure when meeting:design issues are discussed.

[samreid]

Please see phetsims/scenery#639 for code we factored out for this pattern.

[pixelzoom]

Now that we're starting to spread this out across multiple issues, I'm no longer sure what the most recent version of the creator pattern is. Examples:

(1) How is SimpleDragHandler.createForwardingListener used in the pattern? See phetsims/scenery#639 (comment)

(2) When did we add "ignore this if already dragging" to the pattern? See example in phetsims/scenery#639 (comment). Why was it added? Doesn't it prevent creation of a model element until the previously-created model element is released?

[pixelzoom]

Discussed (2) above via Skype. "ignore this if already dragging" is apparently related to touchSnag. Don't snag something if you're already dragging something else.

[pixelzoom]

... Don't snag something if you're already dragging something else.

Um, no, that's not the answer. Snag is a feature of SimpleDragHandler. The example in phetsims/scenery#639 (comment) is a generic input listener. So my question is still open: Why is "ignore this if already dragging" required?

[samreid]

In case it got an event forwarded to it by SimpleDragHandler...?

[samreid]

See phetsims/scenery#639 (comment) for an improved API for SimpleDragHandler.createForwardingListener.

[pixelzoom]

@samreid wrote:

See phetsims/scenery#639 (comment) for an improved API for SimpleDragHandler.createForwardingListener.

I don't think this is sufficient for summarizing the current state of the recommended pattern. Other developers (including some who have never used this pattern, e.g. phetsims/projectile-motion#20 (comment)) are waiting for guidance.

[samreid]

@andrea-phet is going to look into phetsims/projectile-motion#20 (comment) and report back.

[andrealin]

What I'm understanding is that a general createForwardingListener has been created in common code, so I should use that instead of what is happening in projectile motion right now. I will try to follow the instructions in phetsims/scenery#639 (comment) and then ask questions or ask for a review from @jbphet or @samreid .

[andrealin]

I used createForwardingListener in Projectile Motion and it works well. Assigning to @jbphet to review phetsims/projectile-motion#20, and then update here.

[andrealin]

phetsims/projectile-motion#20 has been reviewed and closed, so I think this issue is all good as well. Removing the high priority label, and @jbphet feel free to close.

[jbphet]

This issue has lived a long and productive life, and we have a pattern now for event forwarding that I think we developers are comfortable with. Have a more generalized model creator node is probably not anything that we will do in the near future, but the event forwarding pattern gets us pretty close. I think it's time to close this, but I'd like to clear it with other devs, since this has been closed and reopened once already. Labeling for dev meeting.

[pixelzoom]

8/17/17 dev meeting: Consensus is to close.