PAIR-code / megaplot

Apache License 2.0
19 stars 5 forks source link

`TransitionTimeMs` is not inherited from one callback invocation to the next #76

Open jimbojw opened 1 year ago

jimbojw commented 1 year ago

Inside of a Sprite callback, the API user can set TransitionTimeMs to specify how long the transition should take to complete. However, unlike other sprite attributes, TransitionTimeMs is not inherited from the previous phase and must be set in each phase callback.

Example using Sprite API (see comments):

const scene = new Scene({
  defaultTransitionTimeMs: 400,  // Default transition time is 400.
});

const sprite = scene.createSprite();

sprite
  .enter((s: SpriteView) => {
    s.TransitionTimeMs = 1000;  // Explicitly set time to 1000 (1 sec).
    s.SizeWorld = 1;
    s.FillOpacity = 0;
  })
  .update((s: SpriteView) => {
    // CURRENTLY: s.TransitionTimeMs implicitly set to the default (400).
    // DESIRED: s.TransitionTimeMs should persist the 1000 from enter().

    // s.SizeWorld is correctly inherited from previous state (1).

    s.FillOpacity = 1;
  });

Example using the Selection API (see comments):

const scene = new Scene({
  defaultTransitionTimeMs: 500,  // Default transition time is 500.
});

const selection = scene.createSelection();

selection
  .onInit((s: SpriteView) => {
    // CURRENTLY: Ignored for rendering and not persisted.
    // DESIRED: Still ignore for rendering, but persist value.
    s.TransitionTimeMs = 1000;  

    s.SizeWorld = 1;
    s.FillOpacity = 0;
  })
  .onEnter((s: SpriteView) => {
    // CURRENTLY: s.TransitionTimeMs implicitly set to the default (500).
    // DESIRED: s.TransitionTimeMs should persist 1000 from onInit().

    s.TransitionTimeMs = 2000;  // Explicitly set to 2000.

    // s.SizeWorld is inherited from previous state (1).
    s.FillOpacity = 1;
  })
  .onUpdate((s: SpriteView) => {
    // CURRENTLY: s.TransitionTimeMs implicitly set to the default (500).
    // DESIRED: s.TransitionTimeMs should persist 2000 from onEnter().

    // s.SizeWorld is inherited from previous state (1).
    s.FillOpacity = 1;
  });

Ideally, TransitionTimeMs should persist between callback invocations. The Scene's defaultTransitionTimeMs should only be used until the first time a transition time is needed, and then after that the previous value should be used again. This would make it consistent with other sprite attributes.

Note: The fact that the transition time is ignored for Selection onInit() is intentional. The attributes set for onInit() should be applied before the first frame in which the Sprite is rendered. However, the TransitionTimeMs set during onInit() should be used for callback invocations. Ignoring this value and not persisting it is counterintuitive.