CODAP Case index is incorrect

[KatieWoe]

Device Dell OS Win 11 Browser Chrome Problem Description For https://github.com/phetsims/qa/issues/1096 After a large number of projectiles, it is likely that some of the index numbers will not match the case index numbers. Some will be duplicated, some will be skipped. This happens both with shooting projectiles individually and generating the data all at once.

Visuals

[mattpen]

@KatieWoe - did you mean to assign @matthew-blackman?

[KatieWoe]

I did. Sorry about that.

[KatieWoe]

The graph can be used to visualize this. Seems to happen more as numbers get larger.

[KatieWoe]

Also in https://github.com/phetsims/qa/issues/1099

[samreid]

This fixes the problem in my testing, by queuing the events coming from the addEventListener. They were happening concurrently (see the debug statements)

```diff Subject: [PATCH] Do not show the pool scale when resetting the boat scene, see https://github.com/phetsims/buoyancy/issues/179 --- Index: phet-io-wrappers/common/js/PhetioClient.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/common/js/PhetioClient.ts b/phet-io-wrappers/common/js/PhetioClient.ts --- a/phet-io-wrappers/common/js/PhetioClient.ts (revision d6acbd1986b7195bf48f1752c14e46fb7f62e8cd) +++ b/phet-io-wrappers/common/js/PhetioClient.ts (date 1719257972375) @@ -431,7 +431,7 @@ * Callback for postMessage events from the simulation frame. * @param event - the browser event type, see https://developer.mozilla.org/en-US/docs/Web/API/MessageEvent */ - const windowMessageListener = async ( event: MessageEvent ) => { + const myListener = async ( event: MessageEvent ) => { // Make sure the message came from the frame we are interested in if ( event.source === frame.contentWindow ) { @@ -488,7 +488,34 @@ } }; - // Listen for events from the simulation iframe + const queue: IntentionalAny[] = []; + let isProcessing = false; + + const dequeueAndProcess = async () => { + if ( isProcessing ) { + return; + } // Exit if a processing is already ongoing + if ( queue.length === 0 ) { + return; + } // Exit if no messages are in the queue + + isProcessing = true; + const event = queue.shift(); // Remove the first event from the queue + console.log( 'Pulled from the queue, length:', queue.length); + await myListener( event ); + isProcessing = false; + + // Call recursively to process the next message + await dequeueAndProcess(); + }; + + const windowMessageListener = async event => { + queue.push( event ); // Add incoming event to the queue + console.log( 'Message queued. Queue length:', queue.length ); + await dequeueAndProcess(); // Start processing if not already doing so + }; + +// Listen for events from the simulation iframe window.addEventListener( 'message', windowMessageListener, false ); // Set up beforeunload and unload listeners to record those events to the data stream. Index: projectile-data-lab/js/common/PDLConstants.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/projectile-data-lab/js/common/PDLConstants.ts b/projectile-data-lab/js/common/PDLConstants.ts --- a/projectile-data-lab/js/common/PDLConstants.ts (revision e33aef40c0c837fcf2ea79a9aa80bbe723e777ba) +++ b/projectile-data-lab/js/common/PDLConstants.ts (date 1719254079529) @@ -71,7 +71,7 @@ PIXELS_TO_DISTANCE: PIXELS_TO_DISTANCE, // The factor multiple for time speed in 'Fast' mode - TIME_SPEED_FAST: 6, + TIME_SPEED_FAST: 60, // The projectile source images are scaled by this factor when drawing them on the canvas PROJECTILE_IMAGE_SCALE_FACTOR: 0.15, Index: phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html b/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html --- a/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html (revision 74d368e103dba098023262340b551de06fa9a7e7) +++ b/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html (date 1719256981131) @@ -245,9 +245,13 @@ }, onSimInitialized: async function() { + let isRunningCount = 0; fieldPhetioIDs.forEach( fieldPhetioID => { phetioClient.invoke( `${fieldPhetioID}.projectileLandedEmitter`, 'addListener', [ async projectile => { + isRunningCount++; + console.log( isRunningCount ); + const launcherStandardDeviationAngle = projectile.launcherStandardDeviationAngle; // Call it angleStability @@ -299,11 +303,24 @@ resource: 'dataContext[projectile-data-lab].item', values: [ codapDataObject ] } ); + console.log( 'elements.push' ); + // debugger; elements.push( { projectile: codapDataObject, result: createResult } ); + + // First, query CODAP for the number of existing rows in the 'projectile-data-lab' data context + const result2 = await codapPhoneAsync( { + action: 'get', + resource: 'dataContext[projectile-data-lab].collection[projectile-data].caseCount' + } ); + + console.log( elements.length, result.values, result2.values ); } else { console.error( 'Failed to get caseCount from CODAP' ); } + + isRunningCount--; + console.log( 'done, isRunning count = ' + isRunningCount ) } ] ); } ); ```

@zepumph can @matthew-blackman and I please consult with you about this? Feel free to take a look or I can help when I return.

[matthew-blackman]

@samreid - Noting that your patch fixes the behavior in #337, #338, and #345.

[samreid]

Updated patch from discussion with @zepumph

```diff Subject: [PATCH] Do not show the pool scale when resetting the boat scene, see https://github.com/phetsims/buoyancy/issues/179 --- Index: phet-io-wrappers/common/js/PhetioClient.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/common/js/PhetioClient.ts b/phet-io-wrappers/common/js/PhetioClient.ts --- a/phet-io-wrappers/common/js/PhetioClient.ts (revision d6acbd1986b7195bf48f1752c14e46fb7f62e8cd) +++ b/phet-io-wrappers/common/js/PhetioClient.ts (date 1719267720357) @@ -366,7 +366,7 @@ // rate of O(N) where N is the number of different functions passed used as arguments in PhET-iO API calls. // // The key is a unique identifier for the function, see wrap(). - private handleToFunctionMap = new Map Promise>(); // eslint-disable-line no-spaced-func + private handleToFunctionMap = new Map void>(); // eslint-disable-line no-spaced-func // Keep track of functions transferred to the PhetioClient so we can reference them private functionHandleID = 0; @@ -431,7 +431,7 @@ * Callback for postMessage events from the simulation frame. * @param event - the browser event type, see https://developer.mozilla.org/en-US/docs/Web/API/MessageEvent */ - const windowMessageListener = async ( event: MessageEvent ) => { + const myListener = async ( event: MessageEvent ) => { // Make sure the message came from the frame we are interested in if ( event.source === frame.contentWindow ) { @@ -465,7 +465,7 @@ // dispatch after clearing from the map because we still want the messageFromList removed from the // stack even if dispatching causes an error, see https://github.com/phetsims/phet-io-wrappers/issues/231 - await this.dispatch( message ); + this.dispatch( message ); } else { console.log( 'ignoring message intended for another PhetioClient, my id=', this.id, ` message was intended for ${message.request!.clientID}` ); @@ -488,8 +488,44 @@ } }; + /** + * This is a queueing mechanism for messages that come in from the simulation. This is necessary because the + * messages can come in so rapidly that one callback could be "in the middle" of processing one event when another comes in. + * For instance, a process could be awaiting when another message comes it. This caused a family of bugs in + * https://github.com/phetsims/projectile-data-lab/issues/338 + */ + const eventQueue: MessageEvent[] = []; + let isProcessing = false; + + const processNextEvent = async () => { + if ( isProcessing ) { + + // Exit if a processing is already ongoing + return; + } + if ( eventQueue.length === 0 ) { + + // Exit if no messages are in the queue. This is necessary since the processNextEvent function is called recursively. + return; + } + + isProcessing = true; + const event = eventQueue.shift()!; // Process the first message in the queue + await myListener( event ); + isProcessing = false; + + // Call recursively to process the next message + await processNextEvent(); + }; + + const windowMessageListener = async ( event: MessageEvent ) => { + eventQueue.push( event ); // Add incoming event to the queue + await processNextEvent(); // Start processing if not already doing so + }; + // Listen for events from the simulation iframe - window.addEventListener( 'message', windowMessageListener, false ); + // window.addEventListener( 'message', windowMessageListener, false ); + window.addEventListener( 'message', myListener, false ); // Set up beforeunload and unload listeners to record those events to the data stream. const recordBeforeUnloadListener = () => { @@ -612,7 +648,7 @@ * @param {SimToWrapperMessage} message * @ignore */ - private async dispatch( message: SimToWrapperMessage ): Promise { + private dispatch( message: SimToWrapperMessage ): void { // {SimToWrapperCallback} - The functionHandle field marks that we just need to execute the function with the args. if ( message.type === 'callback' ) { @@ -620,7 +656,8 @@ assert && assert( callbackMessage.functionHandle, 'callbacks must have a functionHandle' ); assert && assert( callbackMessage.args, 'callbacks must have args' ); - await this.handleToFunctionMap.get( callbackMessage.functionHandle! )!( ...callbackMessage.args ); + // Do not await here, because we do not want arbitrary client callbacks holding up the PhetioClient execution. + this.handleToFunctionMap.get( callbackMessage.functionHandle! )!( ...callbackMessage.args ); return; } Index: projectile-data-lab/js/common/PDLConstants.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/projectile-data-lab/js/common/PDLConstants.ts b/projectile-data-lab/js/common/PDLConstants.ts --- a/projectile-data-lab/js/common/PDLConstants.ts (revision e33aef40c0c837fcf2ea79a9aa80bbe723e777ba) +++ b/projectile-data-lab/js/common/PDLConstants.ts (date 1719254079529) @@ -71,7 +71,7 @@ PIXELS_TO_DISTANCE: PIXELS_TO_DISTANCE, // The factor multiple for time speed in 'Fast' mode - TIME_SPEED_FAST: 6, + TIME_SPEED_FAST: 60, // The projectile source images are scaled by this factor when drawing them on the canvas PROJECTILE_IMAGE_SCALE_FACTOR: 0.15, Index: density-buoyancy-common/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts b/density-buoyancy-common/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts --- a/density-buoyancy-common/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts (revision fcac2779775372bb7a48f68dbfc79333c62b4e2e) +++ b/density-buoyancy-common/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts (date 1719246494870) @@ -19,6 +19,18 @@ import { BottleOrBoat, BottleOrBoatValues } from './BottleOrBoat.js'; import StringUnionProperty from '../../../../../axon/js/StringUnionProperty.js'; import MassTag from '../../../common/model/MassTag.js'; +import Basin from '../../../common/model/Basin.js'; +import Mass from '../../../common/model/Mass.js'; + +// Faster than normal stepping to fill the boat (kind of like animation speed) +const FILL_EMPTY_MULTIPLIER = 0.3; + +// 90% of the boat is out of the water before spilling out the full boat +const BOAT_READY_TO_SPILL_OUT_THRESHOLD = 0.9; + +// Y model distance of tolerance between the boat basin fluidY level and the boat basin stepTop. This was needed to +// prevent filling thrashing as a containing mass floats around. See updateLiquidLevel(); +const BOAT_FULL_THRESHOLD = 0.01; export type BuoyancyApplicationsModelOptions = DensityBuoyancyModelOptions; @@ -31,6 +43,9 @@ public readonly boat: Boat; private readonly scale: Scale; // Scale sitting on the ground next to the pool + // Flag that sets an animation to empty the boat of any water inside of it + private spillingWaterOutOfBoat = false; + public constructor( options: BuoyancyApplicationsModelOptions ) { const tandem = options.tandem; @@ -103,6 +118,20 @@ assert && assert( !this.boat.visibleProperty.value || !this.bottle.visibleProperty.value, 'Boat and bottle should not be visible at the same time' ); } ); + + let boatVerticalVelocity = 0; + let boatVerticalAcceleration = 0; + + this.postStepPhase1Emitter.addListener( dt => { + const boat = this.boat; + + if ( dt ) { + boat.setUnderwaterState( this.pool.fluidYInterpolatedProperty.currentValue ); + const nextBoatVerticalVelocity = this.engine.bodyGetVelocity( boat.body ).y; + boatVerticalAcceleration = ( nextBoatVerticalVelocity - boatVerticalVelocity ) / dt; + boatVerticalVelocity = nextBoatVerticalVelocity; + } + } ); } public override step( dt: number ): void { @@ -112,10 +141,6 @@ super.step( dt ); } - public override getBoat(): Boat | null { - return this.boat; - } - /** * Moves the boat and block to their initial locations (see https://github.com/phetsims/buoyancy/issues/25) */ @@ -143,11 +168,152 @@ super.reset(); + this.spillingWaterOutOfBoat = false; + this.sceneProperty.reset(); assert && assert( !this.boat.visibleProperty.value || !this.bottle.visibleProperty.value, 'Boat and bottle should not be visible at the same time' ); } + + private updateFluidsForBoat( poolFluidVolume: number ): number { + const boat = this.boat; + + assert && assert( boat, 'boat needed to update liquids for boat' ); + + const boatBasin = boat.basin; + if ( boat.visibleProperty.value ) { + let boatFluidVolume = boatBasin.fluidVolumeProperty.value; + const boatBasinMaximumVolume = boatBasin.getMaximumVolume( boatBasin.stepTop ); + + const poolEmptyVolumeToBoatTop = this.pool.getEmptyVolume( Math.min( boat.stepTop, this.poolBounds.maxY ) ); + const boatEmptyVolumeToBoatTop = boatBasin.getEmptyVolume( boat.stepTop ); + + // Calculate adjustments to water volumes to match the current space in the basin + let poolExcess = poolFluidVolume - poolEmptyVolumeToBoatTop; + let boatExcess = boatFluidVolume - boatEmptyVolumeToBoatTop; + + const boatHeight = boat.shapeProperty.value.getBounds().height; + + if ( boatFluidVolume ) { + + // If the top of the boat is out of the water past the height threshold, spill the water back into the pool + // (even if not totally full). + if ( boat.stepTop > this.pool.fluidYInterpolatedProperty.currentValue + boatHeight * BOAT_READY_TO_SPILL_OUT_THRESHOLD ) { + this.spillingWaterOutOfBoat = true; + } + } + else { + // If the boat is empty, stop spilling + this.spillingWaterOutOfBoat = false; + } + + // If the boat is out of the water, spill the water back into the pool + if ( this.spillingWaterOutOfBoat ) { + boatExcess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatFluidVolume ); + } + else if ( boatFluidVolume > 0 && + Math.abs( boatBasin.fluidYInterpolatedProperty.currentValue - boatBasin.stepTop ) >= BOAT_FULL_THRESHOLD ) { + // If the boat is neither full nor empty, nor spilling, then it is currently filling up. We will up no matter + // the current water leve or the boat AND no matter the boats position. This is because the boat can only + // ever be full or empty (or animating to one of those states). + + const excess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatBasinMaximumVolume - boatFluidVolume ); // This animates the boat spilling in + poolExcess = excess; + boatExcess = -excess; + } + + if ( poolExcess > 0 && boatExcess < 0 ) { + const transferVolume = Math.min( poolExcess, -boatExcess ); + poolFluidVolume -= transferVolume; + boatFluidVolume += transferVolume; + } + else if ( boatExcess > 0 ) { + // If the boat overflows, just dump the rest in the pool + poolFluidVolume += boatExcess; + boatFluidVolume -= boatExcess; + } + boatBasin.fluidVolumeProperty.value = boatFluidVolume; + } + else { + + // When the boat is hidden (whether via changing scene or by phet-io), move the fluid from the boat basin to the pool. + poolFluidVolume += boatBasin.fluidVolumeProperty.value; + boatBasin.fluidVolumeProperty.value = 0; + } + return poolFluidVolume; + } + + public override overrideSubmergedVolume( mass: Mass, submergedVolume: number ): number { + const boat = this.boat; + if ( mass === boat && boat.isUnderwater ) { + + // Special consideration for when boat is underwater + // Don't count the liquid inside the boat as part of the mass + return boat.volumeProperty.value; + + } + else { + return submergedVolume; + } + } + + public override overrideMassValue( submergedVolume: number, massValue: number ): number { + return submergedVolume * this.boat.materialProperty.value.density; + } + + + /** + * Computes the heights of the main pool liquid (and optionally that of the boat) + * NOTE: This does not call the super class method, as it needs to manage the sequencing of the actions, hence + * there is duplicated code in this implementation. + * + * TODO: Call super.updateFluid and remove the redundancies from this override, see https://github.com/phetsims/density-buoyancy-common/issues/234 + */ + protected override updateFluid(): void { + const boat = this.boat; + + const basins: Basin[] = [ this.pool ]; + if ( boat && boat.visibleProperty.value ) { + basins.push( boat.basin ); + this.pool.childBasin = boat.basin; + } + else { + this.pool.childBasin = null; + } + + this.masses.forEach( mass => mass.updateStepInformation() ); + basins.forEach( basin => { + basin.stepMasses = this.masses.filter( mass => basin.isMassInside( mass ) ); + } ); + + let poolFluidVolume = this.pool.fluidVolumeProperty.value; + + // May need to adjust volumes between the boat/pool if there is a boat + if ( boat ) { + poolFluidVolume = this.updateFluidsForBoat( poolFluidVolume ); + } + + // Check to see if water "spilled" out of the pool, and set the finalized liquid volume + this.pool.fluidVolumeProperty.value = Math.min( poolFluidVolume, this.pool.getEmptyVolume( this.poolBounds.maxY ) ); + + this.pool.computeY(); + boat && boat.basin.computeY(); + + // If we have a boat that is NOT underwater, we'll assign masses into the boat's basin where relevant. Otherwise, + // anything will go just into the pool's basin. + if ( boat && boat.visibleProperty.value && !boat.isUnderwater ) { + this.masses.forEach( mass => { + mass.containingBasin = boat.basin.isMassInside( mass ) ? boat.basin : ( this.pool.isMassInside( mass ) ? this.pool : null ); + } ); + } + else { + this.masses.forEach( mass => { + mass.containingBasin = this.pool.isMassInside( mass ) ? this.pool : null; + } ); + } + } + } densityBuoyancyCommon.register( 'BuoyancyApplicationsModel', BuoyancyApplicationsModel ); \ No newline at end of file Index: joist/js/Sim.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/Sim.ts b/joist/js/Sim.ts --- a/joist/js/Sim.ts (revision 5d3f17d2c6d9220ea0b5bc54bf934582fe77b24e) +++ b/joist/js/Sim.ts (date 1719265707880) @@ -715,6 +715,30 @@ phet.chipper.queryParameters.legendsOfLearning && new LegendsOfLearningSupport( this ).start(); assert && this.auditScreenNameKeys(); + + const testEmitter = new Emitter<[ number ]>( { + parameters: [ + { + phetioType: NumberIO, + name: 'myNumber' + } + ], + tandem: Tandem.GENERAL_MODEL.createTandem( 'testEmitter' ), + phetioDocumentation: 'Test Emitter' + } ); + // testEmitter.emit(); + + let currentEmitterCount = 0; + + animationFrameTimer.addListener( () => { + + if ( currentEmitterCount < 1000 ) { + for ( let i = 0; i < 100; i++ ) { + testEmitter.emit( currentEmitterCount ); + currentEmitterCount++; + } + } + } ); } /** Index: density-buoyancy-common/js/common/model/DensityBuoyancyModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/common/model/DensityBuoyancyModel.ts b/density-buoyancy-common/js/common/model/DensityBuoyancyModel.ts --- a/density-buoyancy-common/js/common/model/DensityBuoyancyModel.ts (revision fcac2779775372bb7a48f68dbfc79333c62b4e2e) +++ b/density-buoyancy-common/js/common/model/DensityBuoyancyModel.ts (date 1719245708909) @@ -18,10 +18,8 @@ import Pool from './Pool.js'; import Scale, { SCALE_WIDTH } from './Scale.js'; import optionize from '../../../../phet-core/js/optionize.js'; -import Boat from '../../buoyancy/model/applications/Boat.js'; import PhysicsEngine, { PhysicsEngineBody } from './PhysicsEngine.js'; import Mass from './Mass.js'; -import Basin from './Basin.js'; import Cuboid from './Cuboid.js'; import TModel from '../../../../joist/js/TModel.js'; import { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js'; @@ -40,16 +38,6 @@ const GROUND_FRONT_Z = POOL_DEPTH / 2; const POOL_BACK_Z = -POOL_DEPTH / 2; -// Faster than normal stepping to fill the boat (kind of like animation speed) -const FILL_EMPTY_MULTIPLIER = 0.3; - -// 90% of the boat is out of the water before spilling out the full boat -const BOAT_READY_TO_SPILL_OUT_THRESHOLD = 0.9; - -// Y model distance of tolerance between the boat basin fluidY level and the boat basin stepTop. This was needed to -// prevent filling thrashing as a containing mass floats around. See updateLiquidLevel(); -const BOAT_FULL_THRESHOLD = 0.01; - export type DensityBuoyancyModelOptions = { usePoolScale?: boolean; } & PickRequired; @@ -74,8 +62,6 @@ private barrierBody: PhysicsEngineBody; protected readonly availableMasses: ObservableArray; - // Flag that sets an animation to empty the boat of any water inside of it - private spillingWaterOutOfBoat = false; public constructor( providedOptions?: DensityBuoyancyModelOptions ) { const options = optionize()( { @@ -203,9 +189,6 @@ mass.interruptedEmitter.emit(); } ); - let boatVerticalVelocity = 0; - let boatVerticalAcceleration = 0; - // The main engine post-step actions, that will determine the net forces applied on each mass. This callback fires // once per "physics engine step", and so results in potentially up to "p2MaxSubSteps" calls per simulation frame // (30 as of writing). This instance lives for the lifetime of the simulation, so we don't need to remove this @@ -216,14 +199,7 @@ // {number} const gravity = this.gravityProperty.value.value; - const boat = this.getBoat(); - - if ( boat && dt ) { - boat.setUnderwaterState( this.pool.fluidYInterpolatedProperty.currentValue ); - const nextBoatVerticalVelocity = this.engine.bodyGetVelocity( boat.body ).y; - boatVerticalAcceleration = ( nextBoatVerticalVelocity - boatVerticalVelocity ) / dt; - boatVerticalVelocity = nextBoatVerticalVelocity; - } + this.postStepListenerPhase1Emitter.emit( dt ); // Will set the force Properties for all the masses this.masses.forEach( mass => { @@ -291,12 +267,8 @@ let massValue = mass.massProperty.value; - if ( mass === boat && boat.isUnderwater ) { - // Special consideration for when boat is underwater - // Don't count the liquid inside the boat as part of the mass - submergedVolume = boat.volumeProperty.value; - massValue = submergedVolume * boat.materialProperty.value.density; - } + submergedVolume = this.overrideSubmergedVolume( submergedVolume ); + massValue = this.overrideMassValue( submergedVolume, massValue ); if ( submergedVolume !== 0 ) { const displacedMass = submergedVolume * this.pool.fluidDensityProperty.value; @@ -352,126 +324,31 @@ this.pool.scale && this.availableMasses.push( this.pool.scale ); } - /** - * Returns the boat (if there is one). Overridden in subclasses that have a boat. - */ - public getBoat(): Boat | null { - return null; + public overrideMassValue( submergedVolume: number, massValue: number ): number { + return massValue; + } + + public overrideSubmergedVolume( submergedVolume: number ): number { + return submergedVolume; } /** * Computes the heights of the main pool liquid (and optionally that of the boat) */ - private updateFluid(): void { - const boat = this.getBoat(); + protected updateFluid(): void { - const basins: Basin[] = [ this.pool ]; - if ( boat && boat.visibleProperty.value ) { - basins.push( boat.basin ); - this.pool.childBasin = boat.basin; - } - else { - this.pool.childBasin = null; - } + this.pool.childBasin = null; this.masses.forEach( mass => mass.updateStepInformation() ); - basins.forEach( basin => { - basin.stepMasses = this.masses.filter( mass => basin.isMassInside( mass ) ); - } ); - - let poolFluidVolume = this.pool.fluidVolumeProperty.value; - - // May need to adjust volumes between the boat/pool if there is a boat - if ( boat ) { - poolFluidVolume = this.updateFluidsForBoat( poolFluidVolume ); - } + this.pool.stepMasses = this.masses.filter( mass => this.pool.isMassInside( mass ) ); // Check to see if water "spilled" out of the pool, and set the finalized liquid volume - this.pool.fluidVolumeProperty.value = Math.min( poolFluidVolume, this.pool.getEmptyVolume( this.poolBounds.maxY ) ); - + this.pool.fluidVolumeProperty.value = Math.min( this.pool.fluidVolumeProperty.value, this.pool.getEmptyVolume( this.poolBounds.maxY ) ); this.pool.computeY(); - boat && boat.basin.computeY(); - // If we have a boat that is NOT underwater, we'll assign masses into the boat's basin where relevant. Otherwise, - // anything will go just into the pool's basin. - if ( boat && boat.visibleProperty.value && !boat.isUnderwater ) { - this.masses.forEach( mass => { - mass.containingBasin = boat.basin.isMassInside( mass ) ? boat.basin : ( this.pool.isMassInside( mass ) ? this.pool : null ); - } ); - } - else { - this.masses.forEach( mass => { - mass.containingBasin = this.pool.isMassInside( mass ) ? this.pool : null; - } ); - } - } - - private updateFluidsForBoat( poolFluidVolume: number ): number { - const boat = this.getBoat()!; - - assert && assert( boat, 'boat needed to update liquids for boat' ); - - const boatBasin = boat.basin; - if ( boat.visibleProperty.value ) { - let boatFluidVolume = boatBasin.fluidVolumeProperty.value; - const boatBasinMaximumVolume = boatBasin.getMaximumVolume( boatBasin.stepTop ); - - const poolEmptyVolumeToBoatTop = this.pool.getEmptyVolume( Math.min( boat.stepTop, this.poolBounds.maxY ) ); - const boatEmptyVolumeToBoatTop = boatBasin.getEmptyVolume( boat.stepTop ); - - // Calculate adjustments to water volumes to match the current space in the basin - let poolExcess = poolFluidVolume - poolEmptyVolumeToBoatTop; - let boatExcess = boatFluidVolume - boatEmptyVolumeToBoatTop; - - const boatHeight = boat.shapeProperty.value.getBounds().height; - - if ( boatFluidVolume ) { - - // If the top of the boat is out of the water past the height threshold, spill the water back into the pool - // (even if not totally full). - if ( boat.stepTop > this.pool.fluidYInterpolatedProperty.currentValue + boatHeight * BOAT_READY_TO_SPILL_OUT_THRESHOLD ) { - this.spillingWaterOutOfBoat = true; - } - } - else { - // If the boat is empty, stop spilling - this.spillingWaterOutOfBoat = false; - } - - // If the boat is out of the water, spill the water back into the pool - if ( this.spillingWaterOutOfBoat ) { - boatExcess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatFluidVolume ); - } - else if ( boatFluidVolume > 0 && - Math.abs( boatBasin.fluidYInterpolatedProperty.currentValue - boatBasin.stepTop ) >= BOAT_FULL_THRESHOLD ) { - // If the boat is neither full nor empty, nor spilling, then it is currently filling up. We will up no matter - // the current water leve or the boat AND no matter the boats position. This is because the boat can only - // ever be full or empty (or animating to one of those states). - - const excess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatBasinMaximumVolume - boatFluidVolume ); // This animates the boat spilling in - poolExcess = excess; - boatExcess = -excess; - } - - if ( poolExcess > 0 && boatExcess < 0 ) { - const transferVolume = Math.min( poolExcess, -boatExcess ); - poolFluidVolume -= transferVolume; - boatFluidVolume += transferVolume; - } - else if ( boatExcess > 0 ) { - // If the boat overflows, just dump the rest in the pool - poolFluidVolume += boatExcess; - boatFluidVolume -= boatExcess; - } - boatBasin.fluidVolumeProperty.value = boatFluidVolume; - } - else { - - // When the boat is hidden (whether via changing scene or by phet-io), move the fluid from the boat basin to the pool. - poolFluidVolume += boatBasin.fluidVolumeProperty.value; - boatBasin.fluidVolumeProperty.value = 0; - } - return poolFluidVolume; + this.masses.forEach( mass => { + mass.containingBasin = this.pool.isMassInside( mass ) ? this.pool : null; + } ); } /** @@ -480,7 +357,6 @@ public reset(): void { this.gravityProperty.reset(); - this.spillingWaterOutOfBoat = false; this.pool.reset(); this.masses.forEach( mass => mass.reset() ); Index: phet-io-wrappers/js/tests/CrossFrameMessageTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/js/tests/CrossFrameMessageTests.ts b/phet-io-wrappers/js/tests/CrossFrameMessageTests.ts --- a/phet-io-wrappers/js/tests/CrossFrameMessageTests.ts (revision d6acbd1986b7195bf48f1752c14e46fb7f62e8cd) +++ b/phet-io-wrappers/js/tests/CrossFrameMessageTests.ts (date 1719265986361) @@ -253,6 +253,47 @@ } } ); +const longRunningThing = async ( number: number ) => { + return new Promise( resolve => { + setTimeout( () => { + resolve( number ); + }, 100 ); + } ); +}; + +QUnit.test( `${simName}: test rapid messaging`, assert => { + + crossFrameMessageScaffolding( assert, async ( iframe: HTMLIFrameElement, phetioClient: PhetioClient, finished: VoidFunction ) => { + try { + + await phetioClient.launchSimulation( { + onSimInitialized: async () => { + + console.log( 'bonjour' ); + + const localePropertyPhetioID = `${phetio.PhetioClient.CAMEL_CASE_SIMULATION_NAME}.general.model.testEmitter`; + + await phetioClient.invokeAsync( localePropertyPhetioID, 'addListener', [ async ( number: number ) => { + console.log( 'received a number from the simulation ' + number ); + + const result = await longRunningThing( number ); + console.log( 'by the way, the number from the sim was: ' + result ); + } ] ); + + setTimeout( finished, 30000 ); + }, + onError( e ) { + throw e; + } + } ); + } + catch( e ) { + finished(); + throw e; + } + } ); +} ); + // factor out sim frame and PhetioClient creation/disposal for each test const crossFrameMessageScaffolding = ( assert: Assert, callback: ( iframe: HTMLIFrameElement, phetioClient: PhetioClient, finished: VoidFunction ) => void ) => { Index: phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html b/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html --- a/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html (revision 74d368e103dba098023262340b551de06fa9a7e7) +++ b/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html (date 1719263806159) @@ -245,8 +245,11 @@ }, onSimInitialized: async function() { + let concurrentCount = 0; + fieldPhetioIDs.forEach( fieldPhetioID => { phetioClient.invoke( `${fieldPhetioID}.projectileLandedEmitter`, 'addListener', [ async projectile => { + concurrentCount++; const launcherStandardDeviationAngle = projectile.launcherStandardDeviationAngle; @@ -304,6 +307,9 @@ else { console.error( 'Failed to get caseCount from CODAP' ); } + + concurrentCount--; + console.log( concurrentCount ); } ] ); } ); ```

@zepumph and I discussed that we want to make the phetio engine nonblocking--we don't want any awaits there. So to solve this problem and #337, #345, we would like to recommend that @matthew-blackman move the queue from the patch into projectileDataWrapper.html at the projectile added listener site. The same queueing strategy will work there.

However, first @zepumph will continue with other issues we discovered in https://github.com/phetsims/phet-io-wrappers/issues/462 before turning this over to @matthew-blackman

[zepumph]

Alright. I have committed some changes and tests over in https://github.com/phetsims/phet-io-wrappers/issues/462. None of that should effect this issue though. It is only important to note our decision above that we do not think that PhET-iO wrapper internals should await for callbacks and listeners to complete.

From this, I made an untested patch adding the queue over to the codap wrapper. Let me know if you want more help with this, but hopefully this patch (or something close to it), just fixes all the same problems as the above patch.

```diff Subject: [PATCH] the perfect fix, forever and always --- Index: phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html b/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html --- a/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html (revision 1f348a10d194b56229b200d83ea1a72315de082f) +++ b/phet-io-sim-specific/repos/projectile-data-lab/wrappers/codap/projectileDataWrapper.html (date 1719274070140) @@ -246,7 +246,7 @@ onSimInitialized: async function() { fieldPhetioIDs.forEach( fieldPhetioID => { - phetioClient.invoke( `${fieldPhetioID}.projectileLandedEmitter`, 'addListener', [ async projectile => { + const handleLandedProjectile = async projectile => { const launcherStandardDeviationAngle = projectile.launcherStandardDeviationAngle; @@ -304,6 +304,36 @@ else { console.error( 'Failed to get caseCount from CODAP' ); } + }; + + /** + * This is a queueing mechanism for messages that come in from the simulation. This is necessary because the + * messages can come in so rapidly that one callback could be "in the middle" of processing one event when another comes in. + * For instance, a process could be awaiting when another message comes it. This caused a family of bugs in + * https://github.com/phetsims/projectile-data-lab/issues/338 + */ + const projectileQueue = []; + let isProcessing = false; + + const processNextProjectile = async () => { + if ( isProcessing || projectileQueue.length === 0 ) { + // Exit if a processing is already ongoing, or if no messages are in the queue. This is necessary since + // the processNextProjectile function is called recursively. + return; + } + + isProcessing = true; + const projectile = projectileQueue.shift(); + await handleLandedProjectile( projectile ); + isProcessing = false; + + // Call recursively to process the next message + await processNextProjectile(); + }; + + phetioClient.invoke( `${fieldPhetioID}.projectileLandedEmitter`, 'addListener', [ async projectile => { + projectileQueue.push( projectile ); // Add incoming projectile to the queue + processNextProjectile(); // Start processing if not already doing so (no-op if already processing) } ] ); } ); Index: projectile-data-lab/js/common/PDLConstants.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/projectile-data-lab/js/common/PDLConstants.ts b/projectile-data-lab/js/common/PDLConstants.ts --- a/projectile-data-lab/js/common/PDLConstants.ts (revision e33aef40c0c837fcf2ea79a9aa80bbe723e777ba) +++ b/projectile-data-lab/js/common/PDLConstants.ts (date 1719268404386) @@ -71,7 +71,7 @@ PIXELS_TO_DISTANCE: PIXELS_TO_DISTANCE, // The factor multiple for time speed in 'Fast' mode - TIME_SPEED_FAST: 6, + TIME_SPEED_FAST: 60, // The projectile source images are scaled by this factor when drawing them on the canvas PROJECTILE_IMAGE_SCALE_FACTOR: 0.15,

[matthew-blackman]

@zepumph the patch looks great, and in my testing solves this problem as well as #337 and #345. Nice work and thanks for your help on this! I can take things from here. Much appreciated.

[zepumph]

@jonathanolson and I discussed the above patch and were a little worried about the async recursion and how it could potentially create too large of a stack for the browser. TLDR: we think you're likely fine, but should definitely test with thousands of new data points within one or two frames just to be sure.

1. Return the recursive call to processNextProjectile instead of awaiting on it. The functionality is the same, but may have simpler error handling in this case under the hood with async/await.
2. @jonathanolson explained to me what "Tail Recursion" was, and how that can optimize a recursive call into a loop when compiled (https://www.geeksforgeeks.org/tail-recursion/). We aren't sure this is a problem though, but it is interesting.
3. Overall we want to make sure to stress test this function with multiple thousands of calls to the queue, and have it not fail. It will also be good to test on a couple different browsers. We used this toy example and it worked well, but double checking in your case seems good. We saw that the stack errored out after a bit more than 4000 recursive calls without an async step to move things out of the stack and into the heap. Note below that even an "immediate" Promise.resolve successfully "breaks" the recursion step.

```js let count = 10000000; const f = async () => { if ( count-- === 0 ) { return; } if ( count % 4000 === 0 ) { // Errored out if this number was 10000 // await new Promise( resolve => setTimeout( resolve, 0 ) ); await Promise.resolve(); } if ( count % 10000 === 0 ) { console.log( count ); } const currentCount = count; // Adding a variable to the closure ensures that each level needs a scope, and can't get optimized away. // A try/catch causes the issue too because each level gets its own error handling. // try { await f(); // } // catch ( e ) { // console.error( currentCount, e ); // } }; f().then( () => console.log( 'done' ) );

[samreid]

@matthew-blackman would you also like to apply the queue to projectile sampling distributions?

[matthew-blackman]

@zepumph and I ran a successful stress test with 10,000 projectiles created in a single frame. The wrapper passed with flying colors, and we are confident that the queueing system is ready for production.

@samreid - Yes, I'm planning to copy all relevant changes from the PDL wrapper to the PSD wrapper. I was going to do it all in one shot once all remaining PDL wrapper issues are resolved.

[matthew-blackman]

https://github.com/phetsims/projectile-data-lab/issues/349 is being used to track the changes from the PDL wrapper that should be copied to the PSD wrapper.

[Nancy-Salpepi]

There is no longer a "Case Index" column, so going to close this issue.