Fix negative zero in monitors

Author: GarboMuffin

src/engine/runtime.js

@@ -23,6 +23,7 @@ const ScratchLinkWebSocket = require('../util/scratch-link-websocket');
 const FontManager = require('./tw-font-manager');
 const fetchWithTimeout = require('../util/fetch-with-timeout');
 const platform = require('./tw-platform.js');
+const {compareImmutableMaps, mergeMaps} = require('../util/tw-immutable-util');
 
 // Virtual I/O devices.
 const Clock = require('../io/clock');
@@ -2577,7 +2578,7 @@ class Runtime extends EventEmitter {
             this._refreshTargets = false;
         }
 
-        if (!this._prevMonitorState.equals(this._monitorState)) {
+        if (!compareImmutableMaps(this._prevMonitorState, this._monitorState)) {
             this.emit(Runtime.MONITORS_UPDATE, this._monitorState);
             this._prevMonitorState = this._monitorState;
         }
@@ -3112,13 +3113,7 @@ class Runtime extends EventEmitter {
         const id = monitor.get('id');
         if (this._monitorState.has(id)) {
             this._monitorState =
-                // Use mergeWith here to prevent undefined values from overwriting existing ones
-                this._monitorState.set(id, this._monitorState.get(id).mergeWith((prev, next) => {
-                    if (typeof next === 'undefined' || next === null) {
-                        return prev;
-                    }
-                    return next;
-                }, monitor));
+                this._monitorState.set(id, mergeMaps(this._monitorState.get(id), monitor));
             return true;
         }
         return false;

src/util/tw-immutable-util.js

@@ -0,0 +1,48 @@
+/**
+ * @fileoverview Utilities for immutable.js objects.
+ */
+
+/**
+ * Determine if two maps have identical keys and content (Object.is equality)
+ * Unlike the regular immutable.js comparison functions, this one considers 0 and -0 to be different.
+ * @param {OrderedMap} a
+ * @param {OrderedMap} b
+ * @returns {boolean} true if a and b have the same keys and values
+ */
+const compareImmutableMaps = (a, b) => {
+    if (a.size !== b.size) {
+        return false;
+    }
+
+    for (const key of a.keys()) {
+        const aValue = a.get(key);
+        const bValue = b.get(key);
+        if (!Object.is(aValue, bValue)) {
+            return false;
+        }
+    }
+
+    return true;
+};
+
+/**
+ * Merge map B into map A. Values of undefined or null in B will default to B.
+ * Unlike the regular immutable.js comparison functions, this one considers 0 and -0 to be different.
+ * @param {OrderedMap} a
+ * @param {OrderedMap} b
+ * @returns {OrderedMap}
+ */
+const mergeMaps = (a, b) => b
+    .filter(value => value === 0)
+    .merge(a)
+    .mergeWith((prev, next) => {
+        if (typeof next === 'undefined' || next === null) {
+            return prev;
+        }
+        return next;
+    }, b);
+
+module.exports = {
+    compareImmutableMaps,
+    mergeMaps
+};

test/fixtures/tw-monitors.sb3

@@ -0,0 +1,74 @@
+const fs = require('node:fs');
+const path = require('node:path');
+const {test} = require('tap');
+const VM = require('../../src/virtual-machine');
+
+test('monitor update', t => {
+    const fixture = path.join(__dirname, '../fixtures/tw-monitors.sb3');
+    const vm = new VM();
+    vm.loadProject(fs.readFileSync(fixture)).then(() => {
+        const variable = vm.runtime.getTargetForStage().lookupVariableByNameAndType('variable', '');
+        const list = vm.runtime.getTargetForStage().lookupVariableByNameAndType('list', 'list');
+
+        const updates = [];
+        vm.runtime.on('MONITORS_UPDATE', monitors => {
+            const values = {};
+            for (const monitor of monitors.values()) {
+                const name = Object.values(monitor.get('params'))[0];
+                const value = monitor.get('value');
+                values[name] = value;
+            }
+            updates.push(values);
+        });
+
+        // Baseline start
+        updates.length = 0;
+        vm.runtime._step();
+        t.equal(updates.length, 1);
+        t.equal(updates[0].variable, 0);
+        t.same(updates[0].list, []);
+
+        // Change variable to 5
+        updates.length = 0;
+        variable.value = 5;
+        vm.runtime._step();
+        t.equal(updates.length, 1);
+        t.equal(updates[0].variable, 5);
+        t.same(updates[0].list, []);
+
+        // Change variable to -0
+        updates.length = 0;
+        variable.value = -0;
+        vm.runtime._step();
+        t.equal(updates.length, 1);
+        t.ok(Object.is(updates[0].variable, -0));
+        t.same(updates[0].list, []);
+
+        // Change variable to 0
+        updates.length = 0;
+        variable.value = 0;
+        vm.runtime._step();
+        t.equal(updates.length, 1);
+        t.ok(Object.is(updates[0].variable, 0));
+        t.same(updates[0].list, []);
+
+        // Replace list entirely
+        updates.length = 0;
+        list.value = [1, 2, 3];
+        vm.runtime._step();
+        t.equal(updates.length, 1);
+        t.equal(updates[0].variable, 0);
+        t.same(updates[0].list, [1, 2, 3]);
+
+        // Append to list
+        updates.length = 0;
+        list.value.push(4);
+        list.value._monitorUpToDate = false;
+        vm.runtime._step();
+        t.equal(updates.length, 1);
+        t.equal(updates[0].variable, 0);
+        t.same(updates[0].list, [1, 2, 3, 4]);
+
+        t.end();
+    });
+});

test/integration/tw_monitor_update.js

@@ -0,0 +1,91 @@
+const {test} = require('tap');
+const {OrderedMap} = require('immutable');
+const {compareImmutableMaps, mergeMaps} = require('../../src/util/tw-immutable-util');
+
+test('compareImmutableMaps', t => {
+    t.ok(compareImmutableMaps(OrderedMap(), OrderedMap()));
+    t.ok(compareImmutableMaps(OrderedMap({
+        a: 'hello'
+    }), OrderedMap({
+        a: 'hello'
+    })));
+    t.ok(compareImmutableMaps(OrderedMap({
+        what: 0,
+        why: 'how'
+    }), OrderedMap({
+        why: 'how',
+        what: 0
+    })));
+
+    t.notOk(compareImmutableMaps(OrderedMap({
+        a: 0
+    }), OrderedMap({
+        a: -0
+    })));
+    t.notOk(compareImmutableMaps(OrderedMap(), OrderedMap({
+        a: 0
+    })));
+    t.notOk(compareImmutableMaps(OrderedMap({
+        a: 0
+    }), OrderedMap()));
+
+    const arr = [];
+    t.ok(compareImmutableMaps(OrderedMap({
+        arr
+    }), OrderedMap({
+        arr
+    })));
+    t.notOk(compareImmutableMaps(OrderedMap({
+        arr: []
+    }), OrderedMap({
+        arr: []
+    })));
+
+    t.end();
+});
+
+test('mergeMaps', t => {
+    t.ok(compareImmutableMaps(mergeMaps(
+        OrderedMap({
+            a: 'hello',
+            b: 'bye',
+            c: 'ok'
+        }),
+        OrderedMap({
+            b: '!!',
+            c: null,
+            d: 'e',
+            e: undefined
+        })
+    ), OrderedMap({
+        a: 'hello',
+        b: '!!',
+        c: 'ok',
+        d: 'e',
+        e: undefined
+    })));
+
+    t.ok(compareImmutableMaps(mergeMaps(
+        OrderedMap({
+            a: 0
+        }),
+        OrderedMap({
+            a: -0
+        })
+    ), OrderedMap({
+        a: -0
+    })));
+
+    t.ok(compareImmutableMaps(mergeMaps(
+        OrderedMap({
+            a: -0
+        }),
+        OrderedMap({
+            a: 0
+        })
+    ), OrderedMap({
+        a: 0
+    })));
+
+    t.end();
+});