Skip to content

Commit

Permalink
fix: Reduce multiple reinitialization on page load
Browse files Browse the repository at this point in the history
  Vue reactivity and parameter updating were interacting poorly. This commit
  protects against this bad interaction in two ways:

  1. Only changes to values that have different string forms than the
     previous value.  This ensures, for example, that a reactive value is not
     modified just because it is not `===` to the apparently "new" value.
  2. Changes the `parameterChanged()` method to be called once for each
     parameter change to `parametersChanged()` so that multiple simultaneous
     changes are bundled into one call.

  With these changes, the startup sequence is more predictable. We hope this
  change will make end-to-end testing significantly more reproducible.

  Also makes Turtle not restart drawing from the beginning of the path on
  a resize, since it is now able to redraw the full path so far without
  difficulty.

  Also fixes the default value for the `highlight` parameter of FactorFence;
  the multiple initializations were masking the prior erroneous value.
  • Loading branch information
gwhitney committed Oct 24, 2024
1 parent fd853e8 commit 532be7c
Show file tree
Hide file tree
Showing 9 changed files with 47 additions and 30 deletions.
Binary file modified e2e/tests/featured.spec.ts-snapshots/BeattyDNA-chromium-linux.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified e2e/tests/featured.spec.ts-snapshots/ThueTrellis-chromium-linux.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
28 changes: 17 additions & 11 deletions src/sequences/Cached.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,9 +349,14 @@ export function Cached<PD extends GenericParamDescription>(desc: PD) {
* cache and reinitialize the sequence so that every element is
* recomputed when queried.
*/
async parameterChanged(name: string) {
await super.parameterChanged(name)
if (name in standardSequenceParameters) {
async parametersChanged(nameList: string[]) {
await super.parametersChanged(nameList)
let needsReset = false
for (const name of nameList) {
if (!(name in standardSequenceParameters)) {
needsReset = true
continue
}
if (name === 'first') {
// recompute last, leaving length
// unchanged if possible
Expand All @@ -367,9 +372,9 @@ export function Cached<PD extends GenericParamDescription>(desc: PD) {
}
// See if we need to rebase the cache
// We have to assume that cache coverage may be
// patchy; all we can rely on is that the closed interval
// from the _previous_ value of `this.first` to
// `this.lastValueCached` is full.
// patchy; all we can rely on is that the closed
// interval from the _previous_ value of `this.first`
// to `this.lastValueCached` is full.
if (
this.first < this.firstValueCached
|| this.first > this.lastValueCached + 1n
Expand Down Expand Up @@ -416,12 +421,13 @@ export function Cached<PD extends GenericParamDescription>(desc: PD) {
}
}
this.refreshParams()
return
}
this.ready = false
await this.valueCachingPromise
await this.factorCachingPromise
this.initialize()
if (needsReset) {
this.ready = false
await this.valueCachingPromise
await this.factorCachingPromise
this.initialize()
}
}
}

Expand Down
35 changes: 20 additions & 15 deletions src/shared/Paramable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -480,16 +480,21 @@ export class Paramable implements ParamableInterface {
const me = this as Record<string, unknown>
props.push(prop)
if (realized[prop] !== me[prop]) {
me[prop] = realized[prop]
changed.push(prop)
// Looks like we might need to change my value of the prop
// However, we only want to do this if the two items
// derealize into different strings:
const derealizer =
typeFunctions[this.params[prop].type].derealize
const myVersion = derealizer(me[prop] as never)
const newVersion = derealizer(realized[prop] as never)
if (newVersion !== myVersion) {
// OK, really have to change
me[prop] = realized[prop]
changed.push(prop)
}
}
}
// We could change the cutoff between individual calls to
// parameterChanged and a single call with `#multiple`, if we ever
// encounter any evidence that would be helpful.
if (changed.length < props.length - 1 || changed.length < 3)
for (const prop of changed) await this.parameterChanged(prop)
else await this.parameterChanged('#multiple')
if (changed.length > 0) await this.parametersChanged(changed)
}
/**
* refreshParams() copies the current values of top-level properties into
Expand Down Expand Up @@ -527,15 +532,15 @@ export class Paramable implements ParamableInterface {
}

/**
* parameterChanged() is called whenever the value of a particular parameter
* is changed. By default, this does nothing, but may be overriden to
* perform any kind of update action for that given parameter.
* parametersChanged() is called whenever the values of one or more
* parameters have changed. (Sometimes multiple parameters change
* simultaneously, as when loading parameters at startup.) By default,
* this method does nothing, but may be overriden to perform any kind of
* update actions for the listed parameters.
*
* Note that if almost all of the parameters have changed at once, this
* function will be called a single time with the name value '#multiple'.
* @param {string} name the name of the parameter which has been changed
* @param {string[]} name the names of one or more parameters that changed
*/
async parameterChanged(_name: string) {
async parametersChanged(_name: string[]) {
return
}

Expand Down
2 changes: 1 addition & 1 deletion src/visualizers/FactorFence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ const paramDesc = {
in the display
**/
highlight: {
default: 1,
default: 1n,
type: ParamType.BIGINT,
displayName: 'Your favourite number',
required: true,
Expand Down
4 changes: 2 additions & 2 deletions src/visualizers/P5Visualizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,8 @@ export function P5Visualizer<PD extends GenericParamDescription>(desc: PD) {
a div that it's already inhabiting, nothing will happen.
*/

async parameterChanged(_name: string) {
await super.parameterChanged(_name)
async parametersChanged(name: string[]) {
await super.parametersChanged(name)
await this.reset()
}

Expand Down
8 changes: 7 additions & 1 deletion src/visualizers/Turtle.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import p5 from 'p5'
import {markRaw} from 'vue'

import {VisualizerExportModule} from './VisualizerInterface'
import {P5GLVisualizer} from './P5GLVisualizer'
import {VisualizerExportModule} from './VisualizerInterface'
import type {ViewSize} from './VisualizerInterface'

import {CachingError} from '@/sequences/Cached'
import type {
Expand Down Expand Up @@ -620,6 +621,11 @@ class Turtle extends P5GLVisualizer(paramDesc) {
}
}

async resized(_toSize: ViewSize) {
this.cursor = 0 // make sure we redraw
return true // we handled it; framework need not do anything
}

mouseWheel(event: WheelEvent) {
super.mouseWheel(event)
this.cursor = 0 // make sure we redraw
Expand Down

0 comments on commit 532be7c

Please sign in to comment.