CT various errors #298
Comments
|
In phetsims/number-play#19 I merged the two paper number implementations by using the paper number nodes from number-play, which were modified with peeled backgrounds and grippy lines. this was needed for a number-play one-off for student interviews. now that we are moving to a different design, i reverted the paper number nodes to as they were in make-a-ten, and the sim is working normally again. |
I looked into the continuing errors on CT today (show above) - haven't quite figured it out yet but I'm getting closer with logging. I can only reproduce with multitouch fuzzing. relevent logging output to show the order of the problem: The next steps are to follow the path of what happens when the paperNumber is removed - is the drag not being cancelled when the corressposding paperNumberNode is removed? (and therefore here's a patch for my current logging: Index: js/common/view/CountingCommonView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/CountingCommonView.js b/js/common/view/CountingCommonView.js
--- a/js/common/view/CountingCommonView.js (revision 8b2dce6567da2b563aa6366807e82a716e1197a7)
+++ b/js/common/view/CountingCommonView.js (date 1625262518295)
@@ -110,6 +110,7 @@
this.addAndDragNumberCallback, this.tryToCombineNumbersCallback );
this.paperNumberNodeMap[ paperNumberNode.paperNumber.id ] = paperNumberNode;
+ paperNumberNode.paperNumber.wasInMap = true;
this.paperNumberLayerNode.addChild( paperNumberNode );
paperNumberNode.attachListeners();
@@ -152,6 +153,7 @@
* @param {PaperNumber} draggedPaperNumber
*/
tryToCombineNumbers( draggedPaperNumber ) {
+ console.log( 'drag ended, looking for: ' + draggedPaperNumber.id + ' in tryToCombineNumbers' );
const draggedNode = this.findPaperNumberNode( draggedPaperNumber );
const draggedNumberValue = draggedPaperNumber.numberValueProperty.value;
const allPaperNumberNodes = this.paperNumberLayerNode.children;
@@ -166,7 +168,7 @@
const droppedNumberValue = droppedPaperNumber.numberValueProperty.value;
if ( ArithmeticRules.canAddNumbers( draggedNumberValue, droppedNumberValue ) ) {
- this.model.collapseNumberModels( this.availableViewBoundsProperty.value, draggedPaperNumber, droppedPaperNumber );
+ this.model.collapseNumberModels( this.availableViewBoundsProperty.value, draggedPaperNumber, droppedPaperNumber, ', from tryToCombineNumbers' );
return; // A bit weird, but no need to relayer or try combining with others?
}
else {
Index: js/common/model/CountingCommonModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/CountingCommonModel.js b/js/common/model/CountingCommonModel.js
--- a/js/common/model/CountingCommonModel.js (revision 8b2dce6567da2b563aa6366807e82a716e1197a7)
+++ b/js/common/model/CountingCommonModel.js (date 1625262569302)
@@ -43,11 +43,13 @@
* @param {PaperNumber} draggedPaperNumber
* @param {PaperNumber} dropTargetNumber
*/
- collapseNumberModels( availableModelBounds, draggedPaperNumber, dropTargetNumber ) {
+ collapseNumberModels( availableModelBounds, draggedPaperNumber, dropTargetNumber, fromWhere ) {
const dropTargetNumberValue = dropTargetNumber.numberValueProperty.value;
const draggedNumberValue = draggedPaperNumber.numberValueProperty.value;
const newValue = dropTargetNumberValue + draggedNumberValue;
+ console.log( 'collapseNumberModels: ' + draggedPaperNumber.id + ', ' + dropTargetNumber.id + fromWhere );
+
let numberToRemove;
let numberToChange;
@@ -64,7 +66,7 @@
}
// Apply changes
- this.removePaperNumber( numberToRemove );
+ this.removePaperNumber( numberToRemove, ', called from collapseNumberModels' );
numberToChange.changeNumber( newValue );
numberToChange.setConstrainedDestination( availableModelBounds, numberToChange.positionProperty.value, false );
}
@@ -85,7 +87,8 @@
*
* @param {PaperNumber} paperNumber
*/
- removePaperNumber( paperNumber ) {
+ removePaperNumber( paperNumber, callLocation ) {
+ console.log( 'removing ' + paperNumber.id + callLocation );
this.paperNumbers.remove( paperNumber );
}
|
I confirmed that the node is being disposed (and even added my own line of calling interruptSubtreeInput on the node, as well as trying to manually call interrupt() on Here is my latest logging output to see the order of what's happening an an updated patch for my logging. Index: js/common/view/CountingCommonView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/CountingCommonView.js b/js/common/view/CountingCommonView.js
--- a/js/common/view/CountingCommonView.js (revision 8b2dce6567da2b563aa6366807e82a716e1197a7)
+++ b/js/common/view/CountingCommonView.js (date 1625764042067)
@@ -110,6 +110,7 @@
this.addAndDragNumberCallback, this.tryToCombineNumbersCallback );
this.paperNumberNodeMap[ paperNumberNode.paperNumber.id ] = paperNumberNode;
+ paperNumberNode.paperNumber.wasInMap = true;
this.paperNumberLayerNode.addChild( paperNumberNode );
paperNumberNode.attachListeners();
@@ -127,7 +128,10 @@
onPaperNumberRemoved( paperNumber ) {
const paperNumberNode = this.findPaperNumberNode( paperNumber );
+ console.log( 'calling interruptSubtreeInput and removing paperNumberNode ' + paperNumber.id );
+
delete this.paperNumberNodeMap[ paperNumberNode.paperNumber.id ];
this.closestDragListener.removeDraggableItem( paperNumberNode );
paperNumberNode.dispose();
+ console.log( 'disposing node for ' + paperNumber.id );
}
@@ -152,6 +156,7 @@
* @param {PaperNumber} draggedPaperNumber
*/
tryToCombineNumbers( draggedPaperNumber ) {
+ console.log( 'drag ended, looking for: ' + draggedPaperNumber.id + ' in tryToCombineNumbers' );
const draggedNode = this.findPaperNumberNode( draggedPaperNumber );
const draggedNumberValue = draggedPaperNumber.numberValueProperty.value;
const allPaperNumberNodes = this.paperNumberLayerNode.children;
@@ -166,7 +171,7 @@
const droppedNumberValue = droppedPaperNumber.numberValueProperty.value;
if ( ArithmeticRules.canAddNumbers( draggedNumberValue, droppedNumberValue ) ) {
- this.model.collapseNumberModels( this.availableViewBoundsProperty.value, draggedPaperNumber, droppedPaperNumber );
+ this.model.collapseNumberModels( this.availableViewBoundsProperty.value, draggedPaperNumber, droppedPaperNumber, ', called from tryToCombineNumbers' );
return; // A bit weird, but no need to relayer or try combining with others?
}
else {
Index: js/common/view/PaperNumberNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/PaperNumberNode.js b/js/common/view/PaperNumberNode.js
--- a/js/common/view/PaperNumberNode.js (revision 8b2dce6567da2b563aa6366807e82a716e1197a7)
+++ b/js/common/view/PaperNumberNode.js (date 1625763337782)
@@ -94,6 +94,7 @@
},
end: ( event, listener ) => {
+ console.log( 'end called on ' + paperNumber.id );
tryToCombineNumbers( this.paperNumber );
paperNumber.endDragEmitter.emit( paperNumber );
}
Index: js/common/model/CountingCommonModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/CountingCommonModel.js b/js/common/model/CountingCommonModel.js
--- a/js/common/model/CountingCommonModel.js (revision 8b2dce6567da2b563aa6366807e82a716e1197a7)
+++ b/js/common/model/CountingCommonModel.js (date 1625763982470)
@@ -43,11 +43,13 @@
* @param {PaperNumber} draggedPaperNumber
* @param {PaperNumber} dropTargetNumber
*/
- collapseNumberModels( availableModelBounds, draggedPaperNumber, dropTargetNumber ) {
+ collapseNumberModels( availableModelBounds, draggedPaperNumber, dropTargetNumber, fromWhere ) {
const dropTargetNumberValue = dropTargetNumber.numberValueProperty.value;
const draggedNumberValue = draggedPaperNumber.numberValueProperty.value;
const newValue = dropTargetNumberValue + draggedNumberValue;
+ console.log( 'in collapseNumberModels: ' + draggedPaperNumber.id + ', ' + dropTargetNumber.id + fromWhere );
+
let numberToRemove;
let numberToChange;
@@ -64,7 +66,7 @@
}
// Apply changes
- this.removePaperNumber( numberToRemove );
+ this.removePaperNumber( numberToRemove, ', called from collapseNumberModels' );
numberToChange.changeNumber( newValue );
numberToChange.setConstrainedDestination( availableModelBounds, numberToChange.positionProperty.value, false );
}
@@ -85,7 +87,8 @@
*
* @param {PaperNumber} paperNumber
*/
- removePaperNumber( paperNumber ) {
+ removePaperNumber( paperNumber, callLocation ) {
+ console.log( 'removing paperNumber ' + paperNumber.id + callLocation );
this.paperNumbers.remove( paperNumber );
} |
Interrupting a pressed DragListener definitely should be calling |
|
@jonathanolson can you see if the above commit makes sense to you? The patch for logging above should give some context, or I'm happy to pair to talk about the problem. But CT is passing now at least. |
|
Reviewed the commit and your fix seems reasonable, however I think I'm confused as to why a drag event is even starting for the problem Node in the first place... For this example: In the above logging 139 is the Node that was being dragged. 149 is the Node that is being removed, but there is no code in that trail that suggests to me that a drag event is triggered for 149. I don't think your fix is breaking anything, and it obviously is fixing this problem, but I have a slight suspicion that a drag event is triggering when perhaps it shouldn't? It may be easier to connect virtually on this @chrisklus, let me know what you think. |
|
This seems like it is probably deferred until make-a-ten becomes a priority again. |
Various errors from common code changes in the last 24 hours - I'll tend to cleanup here as I clean up work in Number Play from the last week.
The text was updated successfully, but these errors were encountered: