phetsims / fourier-making-waves

"Fourier: Making Waves" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 3 forks source link

Missing dependencies for DerivedProperties #239

Closed pixelzoom closed 10 months ago

pixelzoom commented 10 months ago

In https://github.com/phetsims/axon/issues/441, missing dependencies were discovered for some DerivedProperties. Patches are included below. Before pushing these changes, they should be throughly tested to make sure that they do not cause regressions or (more likely) impact performance.

Patch: 2 missing dependency in WavePacketComponentsChart ```diff Subject: [PATCH] add missing dependency, https://github.com/phetsims/axon/issues/441 --- Index: js/wavepacket/model/WavePacketComponentsChart.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/wavepacket/model/WavePacketComponentsChart.ts b/js/wavepacket/model/WavePacketComponentsChart.ts --- a/js/wavepacket/model/WavePacketComponentsChart.ts (revision 81d93506e7dbf3db8828e2d9bb43d03c8adcaade) +++ b/js/wavepacket/model/WavePacketComponentsChart.ts (date 1699487167792) @@ -44,12 +44,12 @@ super( domainProperty, xAxisDescriptionProperty, wavePacket.L, wavePacket.T, tandem ); this.componentDataSetsProperty = new DerivedProperty( - [ wavePacket.componentsProperty, domainProperty, seriesTypeProperty, xAxisDescriptionProperty ], - ( components, domain, seriesType, xAxisDescription ) => { + [ wavePacket.componentsProperty, wavePacket.componentSpacingProperty, domainProperty, seriesTypeProperty, xAxisDescriptionProperty ], + ( components, componentSpacing, domain, seriesType, xAxisDescription ) => { let dataSets: Vector2[][] = EMPTY_DATA_SET; if ( components.length > 0 ) { dataSets = WavePacketComponentsChart.createComponentsDataSets( components, - wavePacket.componentSpacingProperty.value, domain, seriesType, xAxisDescription.range ); + componentSpacing, domain, seriesType, xAxisDescription.range ); } return dataSets; } ); ```
Patch: 6 missing dependences in WavePacketSumChart ```diff Subject: [PATCH] add missing dependency, https://github.com/phetsims/axon/issues/441 --- Index: js/wavepacket/model/WavePacketSumChart.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/wavepacket/model/WavePacketSumChart.ts b/js/wavepacket/model/WavePacketSumChart.ts --- a/js/wavepacket/model/WavePacketSumChart.ts (revision 81d93506e7dbf3db8828e2d9bb43d03c8adcaade) +++ b/js/wavepacket/model/WavePacketSumChart.ts (date 1699487572014) @@ -94,8 +94,8 @@ } ); this.sumDataSetProperty = new DerivedProperty( - [ finiteSumDataSetProperty, infiniteSumDataSetProperty ], - ( finiteDataSet, infiniteDataSet ) => + [ finiteSumDataSetProperty, infiniteSumDataSetProperty, wavePacket.componentSpacingProperty ], + ( finiteDataSet, infiniteDataSet, componentSpacing ) => ( wavePacket.getNumberOfComponents() === Infinity ) ? infiniteDataSet : finiteDataSet ); @@ -105,19 +105,19 @@ // then combining y values. Points are ordered by increasing x value. // This is based on the updateEnvelope method in D2CSumView.js. const finiteWaveformEnvelopeDataSetProperty = new DerivedProperty( - [ this.waveformEnvelopeVisibleProperty, finiteSumDataSetProperty ], - ( waveformEnvelopeVisible, finiteSumDataSet ) => { + [ this.waveformEnvelopeVisibleProperty, finiteSumDataSetProperty, seriesTypeProperty, + wavePacket.componentsProperty, wavePacket.componentSpacingProperty, domainProperty, xAxisDescriptionProperty ], + ( waveformEnvelopeVisible, finiteSumDataSet, seriesType, + wavePacketComponents, wavePacketComponentSpacing, domain, xAxisDescription ) => { let dataSet: Vector2[] = EMPTY_DATA_SET; if ( waveformEnvelopeVisible && finiteSumDataSet.length > 0 ) { // We'll be using finiteSumDataSet as one of the data sets. It was computed for either a SeriesType, // either sine or cosine. Compute the other data set by creating component data sets using the other // SeriesType, then summing those data sets. - const seriesType = ( seriesTypeProperty.value === SeriesType.SIN ) ? SeriesType.COS : SeriesType.SIN; + const otherSeriesType = ( seriesType === SeriesType.SIN ) ? SeriesType.COS : SeriesType.SIN; const otherComponentDataSets = WavePacketComponentsChart.createComponentsDataSets( - wavePacket.componentsProperty.value, wavePacket.componentSpacingProperty.value, domainProperty.value, - seriesType, xAxisDescriptionProperty.value.range - ); + wavePacketComponents, wavePacketComponentSpacing, domain, otherSeriesType, xAxisDescription.range ); const otherSumDataSet = createSumDataSet( otherComponentDataSets ); // Combine the 2 data sets to create the envelope. @@ -149,8 +149,8 @@ } ); this.waveformEnvelopeDataSetProperty = new DerivedProperty( - [ finiteWaveformEnvelopeDataSetProperty, infiniteWaveformEnvelopeDataSetProperty ], - ( finiteDataSet, infiniteDataSet ) => + [ finiteWaveformEnvelopeDataSetProperty, infiniteWaveformEnvelopeDataSetProperty, wavePacket.componentSpacingProperty ], + ( finiteDataSet, infiniteDataSet, componentSpacing ) => ( wavePacket.getNumberOfComponents() === Infinity ) ? infiniteDataSet : finiteDataSet ); ```

Additionally, 1 missing dependency was reported in SumChart for the code shown below. createSumDataSet uses harmonic.ampitudeProperty. But since fourierSeries.amplitudesProperty is derived from the set of harmonic.ampitudeProperty, this is OK.

    this.sumDataSetProperty = new DerivedProperty(
      [ fourierSeries.amplitudesProperty, xAxisDescriptionProperty, domainProperty, seriesTypeProperty, tProperty ],
      ( amplitudes, xAxisDescription, domain, seriesType, t ) =>
        fourierSeries.createSumDataSet( xAxisDescription, domain, seriesType, t )
    );
pixelzoom commented 10 months ago

In https://github.com/phetsims/fourier-making-waves/commit/55308cc8662652f6676ce275989bae02c1910d84, @samreid added accessNonDependencies: true to temporarily silence these problems.

pixelzoom commented 10 months ago

Fixed in the above commits. Closing.