investigate ?listenerOrderRandom
#105
Assignees
Comments
|
Here are some seeds that were reported with failing tests on CT: 70143 I'm testing like: |
zepumph
added a commit
that referenced
this issue
Jul 31, 2023
|
Lots of investigation with @Luisav1 and not a lot of progress. We are just trying to mark the most important, central listenables in the model to see if that fixes things on CT. I'll report back tomorrow. |
zepumph
added a commit
to phetsims/shred
that referenced
this issue
Jul 31, 2023
zepumph
added a commit
that referenced
this issue
Jul 31, 2023
|
This is still showing up on CT this morning: |
|
|
I created an assertion error issue, but we should investigate it here still. looks like none of what I have committed for not shuffling stuff has worked thus far. So we can at least clean that stuff up (unless we need to add instead of replace these changes to get it to work, probably not). |
|
Patch from investigation with @zepumph Subject: [PATCH] Adjust card position, highlight rectangle size, to avoid overlapping objects in highlight mode, see https://github.com/phetsims/center-and-variability/issues/433
---
Index: build-a-nucleus/js/common/view/NucleonNumberPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus/js/common/view/NucleonNumberPanel.ts b/build-a-nucleus/js/common/view/NucleonNumberPanel.ts
--- a/build-a-nucleus/js/common/view/NucleonNumberPanel.ts (revision c4f58a343128da090659473eaca64bd1c183ecc8)
+++ b/build-a-nucleus/js/common/view/NucleonNumberPanel.ts (date 1693000587513)
@@ -68,7 +68,8 @@
panelContents.addChild( newNucleonNumberDisplay );
const oldNucleonNumberProperty = new NumberProperty( nucleonCountProperty.value, {
- validValues: Utils.rangeInclusive( nucleonNumberRange.min, nucleonNumberRange.max )
+ validValues: Utils.rangeInclusive( nucleonNumberRange.min, nucleonNumberRange.max ),
+ hasListenerOrderDependencies: true
} );
// shows the old value of nucleonCountProperty
Index: build-a-nucleus/js/chart-intro/model/DecayEquationModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus/js/chart-intro/model/DecayEquationModel.ts b/build-a-nucleus/js/chart-intro/model/DecayEquationModel.ts
--- a/build-a-nucleus/js/chart-intro/model/DecayEquationModel.ts (revision c4f58a343128da090659473eaca64bd1c183ecc8)
+++ b/build-a-nucleus/js/chart-intro/model/DecayEquationModel.ts (date 1693000041087)
@@ -27,13 +27,16 @@
public constructor( cellModelArray: NuclideChartCellModel[][], protonCountProperty: TReadOnlyProperty<number>, massNumberProperty: TReadOnlyProperty<number> ) {
// keep track of the current nuclide cell to update the decay equation
- this.currentCellModelProperty = new Property( this.getCurrentCellModel( cellModelArray, protonCountProperty.value, massNumberProperty.value ) );
+ this.currentCellModelProperty = new Property( this.getCurrentCellModel( cellModelArray, protonCountProperty.value, massNumberProperty.value ),{
+ hasListenerOrderDependencies: true
+ } );
// initialize to 0
this.finalProtonNumberProperty = new NumberProperty( 0,
- { validValues: Utils.rangeInclusive( 0, BANConstants.CHART_MAX_NUMBER_OF_PROTONS ) } );
+ { validValues: Utils.rangeInclusive( 0, BANConstants.CHART_MAX_NUMBER_OF_PROTONS ),
+ hasListenerOrderDependencies: true} );
this.finalMassNumberProperty = new NumberProperty( 0,
- { validValues: Utils.rangeInclusive( 0, BANConstants.CHART_MAX_NUMBER_OF_PROTONS + BANConstants.CHART_MAX_NUMBER_OF_NEUTRONS ) } );
+ { validValues: Utils.rangeInclusive( 0, BANConstants.CHART_MAX_NUMBER_OF_PROTONS + BANConstants.CHART_MAX_NUMBER_OF_NEUTRONS ),hasListenerOrderDependencies: true } );
// update the finalProtonNumber, finalMassNumber, and the currentCellModel
massNumberProperty.link( () => {
Index: build-a-nucleus/js/chart-intro/model/ChartIntroModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus/js/chart-intro/model/ChartIntroModel.ts b/build-a-nucleus/js/chart-intro/model/ChartIntroModel.ts
--- a/build-a-nucleus/js/chart-intro/model/ChartIntroModel.ts (revision c4f58a343128da090659473eaca64bd1c183ecc8)
+++ b/build-a-nucleus/js/chart-intro/model/ChartIntroModel.ts (date 1693000041090)
@@ -48,7 +48,9 @@
// this is the mini-nucleus that updates based on the particleAtom
this.miniParticleAtom = new ParticleAtom();
- this.selectedNuclideChartProperty = new Property<SelectedChartType>( 'partial' );
+ this.selectedNuclideChartProperty = new Property<SelectedChartType>( 'partial',{
+ hasListenerOrderDependencies: true
+ } );
this.decayEquationModel = new DecayEquationModel( ChartIntroModel.cellModelArray, this.particleNucleus.protonCountProperty, this.particleNucleus.massNumberProperty );
}
Index: build-a-nucleus/js/chart-intro/model/ParticleNucleus.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus/js/chart-intro/model/ParticleNucleus.ts b/build-a-nucleus/js/chart-intro/model/ParticleNucleus.ts
--- a/build-a-nucleus/js/chart-intro/model/ParticleNucleus.ts (revision c4f58a343128da090659473eaca64bd1c183ecc8)
+++ b/build-a-nucleus/js/chart-intro/model/ParticleNucleus.ts (date 1693000041094)
@@ -55,8 +55,12 @@
public readonly modelViewTransform = BANConstants.NUCLEON_ENERGY_LEVEL_ARRAY_MVT;
// keep track of bound levels
- private readonly protonsLevelProperty = new EnumerationProperty( EnergyLevelType.N_ZERO );
- private readonly neutronsLevelProperty = new EnumerationProperty( EnergyLevelType.N_ZERO );
+ private readonly protonsLevelProperty = new EnumerationProperty( EnergyLevelType.N_ZERO,{
+ hasListenerOrderDependencies: true
+ } );
+ private readonly neutronsLevelProperty = new EnumerationProperty( EnergyLevelType.N_ZERO,{
+ hasListenerOrderDependencies: true
+ } );
public constructor() {
super();
Index: axon/js/TinyEmitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TinyEmitter.ts b/axon/js/TinyEmitter.ts
--- a/axon/js/TinyEmitter.ts (revision e87e8b6ce0bc0132cced505878c63a25118c2a17)
+++ b/axon/js/TinyEmitter.ts (date 1692999406313)
@@ -70,6 +70,7 @@
this.hasListenerOrderDependencies = hasListenerOrderDependencies;
}
+ // Listener order is preserved in Set
this.listeners = new Set();
this.emitContexts = [];
Index: build-a-nucleus/js/common/model/BANModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus/js/common/model/BANModel.ts b/build-a-nucleus/js/common/model/BANModel.ts
--- a/build-a-nucleus/js/common/model/BANModel.ts (revision c4f58a343128da090659473eaca64bd1c183ecc8)
+++ b/build-a-nucleus/js/common/model/BANModel.ts (date 1693000154169)
@@ -42,8 +42,12 @@
public readonly neutronNumberRange: Range;
// array of particles sent to the nucleus but not there yet
- public readonly incomingProtons = createObservableArray<BANParticle>();
- public readonly incomingNeutrons = createObservableArray<BANParticle>();
+ public readonly incomingProtons = createObservableArray<BANParticle>({
+ hasListenerOrderDependencies: true
+ });
+ public readonly incomingNeutrons = createObservableArray<BANParticle>({
+ hasListenerOrderDependencies: true
+ });
// If there are any incoming particles currently
public readonly hasIncomingParticlesProperty: TReadOnlyProperty<boolean>;
@@ -51,8 +55,12 @@
// keep track of any particle related animations that may need to be cancelled at some point
public readonly particleAnimations = createObservableArray<Animation | null>();
- public readonly userControlledProtons = createObservableArray<BANParticle>();
- public readonly userControlledNeutrons = createObservableArray<BANParticle>();
+ public readonly userControlledProtons = createObservableArray<BANParticle>({
+ hasListenerOrderDependencies: true
+ });
+ public readonly userControlledNeutrons = createObservableArray<BANParticle>({
+ hasListenerOrderDependencies: true
+ });
// array of all emitted particles, this helps keep track of particles that are no longer "counted" in the atom
public readonly outgoingParticles = createObservableArray<BANParticle>();
@@ -72,7 +80,9 @@
this.hasIncomingParticlesProperty = new DerivedProperty( [
this.incomingProtons.lengthProperty,
this.incomingNeutrons.lengthProperty
- ], ( protonsLength, neutronsLength ) => protonsLength > 0 || neutronsLength > 0 );
+ ], ( protonsLength, neutronsLength ) => protonsLength > 0 || neutronsLength > 0,{
+ hasListenerOrderDependencies: true
+ } );
this.particleAnimations.addItemRemovedListener( animation => {
animation && animation.stop();
@@ -83,13 +93,17 @@
// the stability of the nuclide is determined by the given number of protons and neutrons
this.isStableProperty = new DerivedProperty( [ this.particleAtom.protonCountProperty, this.particleAtom.neutronCountProperty ],
- ( protonNumber, neutronNumber ) => AtomIdentifier.isStable( protonNumber, neutronNumber )
+ ( protonNumber, neutronNumber ) => AtomIdentifier.isStable( protonNumber, neutronNumber ),{
+ hasListenerOrderDependencies: true
+ }
);
// If a nuclide with a given number of protons and neutrons exists.
this.nuclideExistsProperty = new DerivedProperty(
[ this.particleAtom.protonCountProperty, this.particleAtom.neutronCountProperty ],
- ( protonNumber, neutronNumber ) => AtomIdentifier.doesExist( protonNumber, neutronNumber )
+ ( protonNumber, neutronNumber ) => AtomIdentifier.doesExist( protonNumber, neutronNumber ),{
+ hasListenerOrderDependencies: true
+ }
);
const userControlledListener = ( isUserControlled: boolean, particle: Particle ) => {
Index: build-a-nucleus/js/decay/model/DecayModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus/js/decay/model/DecayModel.ts b/build-a-nucleus/js/decay/model/DecayModel.ts
--- a/build-a-nucleus/js/decay/model/DecayModel.ts (revision c4f58a343128da090659473eaca64bd1c183ecc8)
+++ b/build-a-nucleus/js/decay/model/DecayModel.ts (date 1693000587507)
@@ -65,7 +65,9 @@
else {
return halfLife;
}
- }
+ },{
+ hasListenerOrderDependencies: true
+ }
);
// function which would return whether a given nuclide (defined by the number of protons and neutrons) has a certain
Index: build-a-nucleus/js/build-a-nucleus-main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus/js/build-a-nucleus-main.ts b/build-a-nucleus/js/build-a-nucleus-main.ts
--- a/build-a-nucleus/js/build-a-nucleus-main.ts (revision c4f58a343128da090659473eaca64bd1c183ecc8)
+++ b/build-a-nucleus/js/build-a-nucleus-main.ts (date 1693000515131)
@@ -36,4 +36,6 @@
new ChartIntroScreen()
], simOptions );
sim.start();
-} );
\ No newline at end of file
+} );
+
+console.log('bonjour');
\ No newline at end of file
Index: axon/js/createObservableArray.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/createObservableArray.ts b/axon/js/createObservableArray.ts
--- a/axon/js/createObservableArray.ts (revision e87e8b6ce0bc0132cced505878c63a25118c2a17)
+++ b/axon/js/createObservableArray.ts (date 1692998448978)
@@ -134,7 +134,8 @@
const lengthProperty = new NumberProperty( 0, combineOptions<NumberPropertyOptions>( {
numberType: 'Integer',
tandem: options.tandem?.createTandem( 'lengthProperty' ),
- phetioReadOnly: true
+ phetioReadOnly: true,
+ hasListenerOrderDependencies: options.hasListenerOrderDependencies
}, options.lengthPropertyOptions ) );
// The underlying array which is wrapped by the Proxy
Index: build-a-nucleus/js/common/view/NucleonArrowButtons.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus/js/common/view/NucleonArrowButtons.ts b/build-a-nucleus/js/common/view/NucleonArrowButtons.ts
--- a/build-a-nucleus/js/common/view/NucleonArrowButtons.ts (revision c4f58a343128da090659473eaca64bd1c183ecc8)
+++ b/build-a-nucleus/js/common/view/NucleonArrowButtons.ts (date 1693000154174)
@@ -69,7 +69,7 @@
// Disable all buttons if the nuclide doesn't exist and one of the two cases:
// Something is being user controlled OR Particle Atom is not empty
if ( !doesNuclideExist &&
- ( model.particleAtom.massNumberProperty.value !== 0 || userControlledNucleonNumber !== 0 ) ) {
+ ( (protonNumber + neutronNumber) !== 0 || userControlledNucleonNumber !== 0 ) ) {
// disable all arrow buttons if the nuclide does not exist
NucleonArrowButtons.toggleCreatorNodeEnabled( protonsCreatorNode, false );
@@ -112,6 +112,8 @@
firstTypeNotAtRangeBound && this.isNucleonNumberNotAtRangeBounds( direction, secondParticleType, protonNumber, neutronNumber ) :
firstTypeNotAtRangeBound;
}
+ },{
+ hasListenerOrderDependencies: true
} );
};
|
zepumph
added a commit
to phetsims/axon
that referenced
this issue
Aug 25, 2023
zepumph
added a commit
to phetsims/shred
that referenced
this issue
Aug 25, 2023
zepumph
added a commit
that referenced
this issue
Aug 25, 2023
|
@samreid really helped me get into the greater picture of the problem. He had us going through every Property in the sim and adding this flag. That was a great start but then we did that for everything in BAN and it still didn't fix the problem. Next I wondered if it even was a property, so I toggled the default in ReadOnlyProperty and successfully stopped the error. Next I worked at trying to narrow things down further, and a nice separation I created was Properties made on construction vs dynamic ones. Using this patch, I discovered that it is actually triggered by a Property that is created after sim construction: Subject: [PATCH] cleanup listener order temp code, https://github.com/phetsims/build-a-nucleus/issues/105
---
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision 4f571cbc738bdcba5f9f49050c3f2a1eddde9110)
+++ b/axon/js/ReadOnlyProperty.ts (date 1693006332606)
@@ -122,7 +122,7 @@
const options = optionize<PropertyOptions<T>, SelfOptions, PhetioObjectOptions>()( {
units: null,
reentrant: false,
- hasListenerOrderDependencies: false,
+ hasListenerOrderDependencies: window.hi && ( new Error().stack.includes( 'BAN' ) || new Error().stack.includes( 'Particle' ) ),
// phet-io
phetioOuterType: ReadOnlyProperty.PropertyIO,
Index: build-a-nucleus/js/build-a-nucleus-main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus/js/build-a-nucleus-main.ts b/build-a-nucleus/js/build-a-nucleus-main.ts
--- a/build-a-nucleus/js/build-a-nucleus-main.ts (revision 426c80146c6c0b5cfaf3a0bffaa4fad75c1eb1c4)
+++ b/build-a-nucleus/js/build-a-nucleus-main.ts (date 1693006332591)
@@ -35,5 +35,7 @@
new DecayScreen(),
new ChartIntroScreen()
], simOptions );
+ window.hi = true;
+
sim.start();
} );
\ No newline at end of file
From there I was able to see that likely it was coming from Particle. I then traced it down to what I think is the Happy weekend all! |
zepumph
added a commit
to phetsims/shred
that referenced
this issue
Aug 25, 2023
|
phetsims/shred@c6f44ab did in fact fix this. I would like to discuss this dependency more with @Luisav1. I wonder if it will end up relating to #184. |
This was referenced Aug 28, 2023
zepumph
added a commit
that referenced
this issue
Aug 30, 2023
zepumph
added a commit
that referenced
this issue
Aug 30, 2023
|
@Luisav1 and I spent some good time trying to track down the order dependency with the userControlledProperty, while we found some helpful side issues, we didn't get to the bottom of things, and ended up giving up and keeping the workaround in place. I tried to write the assertion the above TODO talked about, but it doesn't work because we use the particle field to "mark" incoming particles before they get added to the ParticleNucleus. This algorithm makes it quite hard to know if a stray particle is set or if it is expected and incoming, but so far it works pretty well! We feel ready to close this issue. Everything is handled in side issues. |
|
Unfortunately this caused a bit more trouble on CT. Reopening. |
zepumph
added a commit
that referenced
this issue
Aug 31, 2023
|
CT has cleared up again. We are ready to re-close. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is showing up on CT. I'll take a first pass:
The text was updated successfully, but these errors were encountered: