Closed samreid closed 4 years ago
drag events were getting called 2x as often as animation frames. Is that normal?
Pretty much yes. Mouse/touch moves can definitely fire at a faster rate than the frame rate. This is generally nice, as it gives a finer-resolution curve of where the pointer went.
It seems like it has performance implications we may need to address.
Depending on the simulation, it's usually better for maintenance when the view is updated for every user event (as necessary), but definitely for performance-critical parts it can be very helpful to delay expensive operations to the view step. Sometimes this is more convenient anyways (e.g. CanvasNode). Usually would use either the "things change every frame" strategy (e.g. RewardNode, particles, etc.), or the "dirty flag when something changes, change view only if it is dirty" method.
Thanks @jonathanolson, it is good to know. For CCK, I've batched some expensive operations to run in the view step. Not sure if we need to do anything else for this issue, not sure if others are aware of this behavior (though it may be good to know when performance optimizing for iPad2).
@pixelzoom thoughts?
Mouse/touch moves can definitely fire at a faster rate than the frame rate. This is generally nice, as it gives a finer-resolution curve of where the pointer went.
It may give finer resolution. But since the view is updated only once per frame, what is the benefit of having the sim process more the 1 drag event per frame? E.g. if N drag events occur during a frame, would the user see anything different if the sim received only the last of those drag events?
E.g. if N drag events occur during a frame, would the user see anything different if the sim received only the last of those drag events?
For simplistic drags, this would be the case. However there are many other cases:
If we're concerned about general performance for this, maybe it would be appropriate to bake in a "lazy" approach to drag handlers, where they ignore certain moves in a frame (and have a step() function that makes them tick). (This approach does have some tricky side-effects, where if the object isn't "dragging" with a pointer, it will probably be receiving a lot more enter/exit events)
For sims where there's no benefit to finer granularity (which I suspect is the majority of sims), it would be unfortunate to have to deal with this is client code in order to hit performance goals. Perhaps a scenery option that allows you to specify whether you want all events or just the "last" event per frame?
Also, my original report was for desktop Chrome--we should check the ratio of input events per frame on iPad2 before optimizing this.
For sims where there's no benefit to finer granularity ...
E.g. for function-builder, testing flagged performance as "OK" on iPad2, and "Poor" on iPad3 (see https://docs.google.com/spreadsheets/d/1crnipxdR5PEn8QBWQKMzPSA6sUMWdaBLRL0j8ZEzvEo/edit#gid=2). In function-builder, "performance" is solely drag performance, and drag handlers are the only performance-critical code. Had I known that multiple drags were being processed per frame, I likely could have improved performance, since this sim has no need for the granularity provided by multiple drag events per frame.
For sims where there's no benefit to finer granularity
I thought the preference now was for most sims to include touch-snaggability, so there would be some benefit to most sims?
Wouldn't my proposal above (that handles this in the drag handler, so it can choose to ignore unneeded events and not disable it for the entire sim) be preferable?
we should check the ratio of input events per frame on iPad2 before optimizing this.
Agreed. I'll create a quick test that can be browsed without having to enable the console.
"Poor" on iPad3
The iPad 3 I thought had at least as much CPU, but many more pixels, so graphical operations would presumably be the bottleneck there?
Pushed a test that can be loaded by visiting scenery/tests/frames-and-events.html
Every animation frame or move event adds (or changes) the next square. Blue is an animation frame, red is a move event.
Chrome canary example showing my moving my mouse (I get 1 event per frame whem moving):
iPad 2 test (Lave) shows predominantly 2 events per frame.
My normal desktop Chrome (Windows 7, Chrome 59) sends a lot of events:
OS X chrome sends 2 events per frame. OS X Safari sends ~4 events per frame.
@jonathanolson asked:
Wouldn't my proposal above (that handles this in the drag handler, so it can choose to ignore unneeded events and not disable it for the entire sim) be preferable?
Yep. That's what I meant by "scenery option that allows you to specify whether you want all events or just the "last" event per frame" -- something baked into common code. To further clarify...
@samreid said:
For CCK, I've batched some expensive operations to run in the view step.
... and that's what I meant by "it would be unfortunate to have to deal with this is client code".
I'll plan to look into modifications we could use for SimpleDragHandler (that's predominantly used for almost all drags).
If that option is provided, presumably it would be required to step() the drag handler?
Also, note that the above tests weren't under heavy load. If that's the case, it's possible that it would fall back to 1 event per frame.
If that option is provided, presumably it would be required to step() the drag handler?
I'm not clear on this. Can you provide a hypothetical example?
I'm not clear on this. Can you provide a hypothetical example?
this.inputListener = new SimpleDragHandler( { ... } );
node.addInputListener( inputListener );
// ...
step: function( dt ) {
this.inputListener.step(); // or another function name, since it has no arg?
}
I'm not clear on this. Can you provide a hypothetical example?
If I understand correctly, this is because scenery would have no way of knowing which input event is the last one so would need an explicit step
call so it could activate the last event.
If I understand correctly, this is because scenery would have no way of knowing which input event is the last one so would need an explicit step call so it could activate the last event.
Correct. There may be potential to have a listener subtype in Joist that hooks this into the sim step, so that manually stepping isn't done.
Tagging for developer meeting, as the proposed way of handling this could use wider discussion.
There may be potential to have a listener subtype in Joist that hooks this into the sim step
That sounds like it is worth investigation.
My vote is to provide this feature for DragListener (next gen) and punt on SimpleDragHandler.
I agree it would be good feature for next gen DragListener so that it has an option to run one per frame or browser-default per frame.
Will be investigated as part of https://github.com/phetsims/scenery/issues/131
Implemented basic support for this above, where you can pass collapseDragEvents:true
and then call step()
on the listener.
If we want to figure out better ways of setting up stepping of listeners (globally, hooking in, etc.), that should be discussed here.
Tested with interaction with the following playground code:
var rect1 = new scenery.Rectangle( 0, 0, 100, 50, { fill: 'red' } );
var rect2 = new scenery.Rectangle( 0, 0, 100, 50, { fill: 'blue', y: 150 } );
var listener1 = new scenery.DragListener( {
translateNode: true,
drag: () => console.log( 'drag1' )
} );
var listener2 = new scenery.DragListener( {
translateNode: true,
collapseDragEvents: true,
drag: () => console.log( 'drag2' )
} );
rect1.addInputListener( listener1 );
rect2.addInputListener( listener2 );
scene.addChild( rect1 );
scene.addChild( rect2 );
display.initializeEvents();
( function step() {
requestAnimationFrame( step );
listener2.step();
console.log( 'update' );
display.updateDisplay();
} )();
With the above code, on MacOS Chrome I was getting 2 drags in-between steps for the red rectangle (1), and 1 drag in-between steps for the blue rectangle (2) with the new options set.
@samreid and @pixelzoom, should we discuss fancier hooks to stepping the listener, and/or does the above change work for you?
Wow, fun reviewing comments from 2+ years ago! I had no recollection whatsoever of this issue. Who is this @pixelzoom guy? :)
The collapseDragEvents
option you've added seems sufficient to me. I can't identify a need to get fancier.
Can you add documentation of when it is appropriate to specify that option?
Added documentation, good to close?
Looks good, thanks! Closing.
In https://github.com/phetsims/circuit-construction-kit-dc/issues/74 I noticed that drag events were getting called 2x as often as animation frames. Is that normal? It seems like it has performance implications we may need to address.
For instance, in @pixelzoom's Function Builder, I put a console.log in stepSimulation and MovableObject.drag and saw this output on Mac chrome:
Note that one occurrence even had 3 drags between animation frames.