Camera: a fade always calls onComplete before it is removed

packages/melonjs/CHANGELOG.md

@@ -25,6 +25,7 @@
 - UI: `UITextButton#bitmapText`, for `label`
 
 ### Fixed
+- Camera: `fadeIn()` / `fadeOut()` (and a `FadeEffect`, so the fade state transition and a fading level trigger too) always call `onComplete`. The fade counted as done once its colour's alpha read as the target, which a 32-bit float can do one step before the tween ends; the camera then removed the effect, stopping the tween before it called `onComplete`, so code waiting on it stayed stuck. The fade now ends when its tween does ([#1810](https://github.com/melonjs/melonJS/issues/1810))
 - Rendering: on WebGL and WebGPU, a clip inside a clip is the overlap of the two, as it already was on Canvas. A clipping container nested in another, or a scrolling `UIPanel` in a clipping parent, drew past the outer clip wherever it stuck out of it, because the inner `clipRect` replaced the outer scissor ([#1816](https://github.com/melonjs/melonJS/issues/1816)). The clipping example shows the case
 - UI: a radio group holds one choice from when its checkboxes are made, not only once they are in the scene: before, several could be checked at once until they were added, and `UICheckbox.checkedIn()` answered `undefined`. A checkbox leaves its group when destroyed; groups hold their members weakly, so one dropped without being destroyed can still be collected
 - UI: `UICheckbox#toggle()` on a checked radio button leaves it checked, as a click does, instead of leaving its group with nothing checked

packages/melonjs/src/camera/effects/fade_effect.ts

@@ -41,7 +41,7 @@ export default class FadeEffect extends CameraEffect {
 	direction: "in" | "out";
 
 	/**
-	 * target alpha value for completion check
+	 * the alpha the fade ends on
 	 * @ignore
 	 * @internal
 	 */
@@ -103,10 +103,12 @@ export default class FadeEffect extends CameraEffect {
 	}
 
 	override update(): void {
-		// check if the tween has reached its target
-		if (this.direction === "in" && this.color.alpha >= this._targetAlpha) {
-			this.isComplete = true;
-		} else if (this.direction === "out" && this.color.alpha <= 0.0) {
+		// Over when the tween is: it has run its course and called
+		// `onComplete`. Not when the alpha reads as the target: the colour
+		// stores it as a 32-bit float, so a value one step short of the end
+		// can already round to it, and the camera then destroyed the effect,
+		// stopping the tween before it ever called `onComplete` (#1810).
+		if (this.tween !== undefined && !this.tween._isRunning) {
 			this.isComplete = true;
 		}
 	}

packages/melonjs/tests/camera.spec.js

@@ -1,4 +1,4 @@
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 import {
 	Application,
 	boot,
@@ -1473,6 +1473,57 @@ describe("Camera2d", () => {
 			expect(called).toEqual(true);
 		});
 
+		it("a fade calls onComplete before the camera removes it, whatever the float rounding (#1810)", () => {
+			// frame by frame as the game loop runs it: the tween steps, then the
+			// camera updates its effects. The alpha is stored as a 32-bit float,
+			// so on some durations it reads as the target one step before the
+			// tween ends; the effect must still wait for the tween
+			const frame = 1000 / 60;
+			const missed = [];
+			for (const direction of ["in", "out"]) {
+				for (let duration = 100; duration <= 1000; duration += 25) {
+					const { camera } = setup();
+					const onComplete = vi.fn();
+					const fade = new FadeEffect(camera, {
+						color: "#ffffff",
+						duration,
+						direction,
+						onComplete,
+					});
+					camera.addCameraEffect(fade);
+					const tween = fade.tween;
+					for (let i = 0; i < 200 && camera.getCameraEffect(FadeEffect); i++) {
+						tween.update(frame);
+						camera.update(frame);
+					}
+					expect(camera.getCameraEffect(FadeEffect)).toBeUndefined();
+					if (onComplete.mock.calls.length !== 1) {
+						missed.push(`${direction} ${duration}ms`);
+					}
+				}
+			}
+			expect(missed).toEqual([]);
+		});
+
+		it("a fade stays until its tween ends, even once its alpha reads as the target", () => {
+			const { camera } = setup();
+			const onComplete = vi.fn();
+			const fade = camera.addCameraEffect(
+				new FadeEffect(camera, { color: "#000", duration: 300, onComplete }),
+			);
+			// the colour already at the target, the tween not yet over
+			fade.color.alpha = 1;
+			camera.update(16);
+			expect(camera.getCameraEffect(FadeEffect)).toBe(fade);
+			expect(onComplete).not.toHaveBeenCalled();
+			// to its end exactly: a finished tween leaves the game loop, so
+			// it is not stepped again
+			advanceTween(fade.tween, 300);
+			camera.update(16);
+			expect(onComplete).toHaveBeenCalledTimes(1);
+			expect(camera.getCameraEffect(FadeEffect)).toBeUndefined();
+		});
+
 		it("fadeIn() convenience method should add a FadeEffect", () => {
 			const { camera } = setup();
 			camera.fadeIn("#000", 500);

packages/melonjs/tests/fade-oncomplete.spec.js

@@ -0,0 +1,117 @@
+/**
+ * `viewport.fadeIn()` / `fadeOut()` always call `onComplete` (#1810).
+ *
+ * Driven the way a game runs: each frame is a `TICK`, as the browser loop
+ * emits it. The application updates first (its stage updates the camera,
+ * which removes the effects that are complete), then the fade's tween steps.
+ *
+ * A fade used to count as complete once its colour's alpha read as the
+ * target. The alpha is a 32-bit float, so on some durations it rounds to
+ * the target one step before the tween ends; the camera then removed the
+ * fade and stopped the tween before it called `onComplete`. Code waiting on
+ * the callback, a scene change say, never ran.
+ */
+import { beforeAll, describe, expect, it, vi } from "vitest";
+import { Application, boot, event, FadeEffect, video } from "../src/index.js";
+
+const FRAME = 1000 / 60;
+
+let app;
+// the clock the frames are stamped with, moved on by every frame
+let now;
+
+/** run one frame of the game loop at 60 fps */
+const tick = () => {
+	now += FRAME;
+	event.emit(event.TICK, now);
+};
+
+/**
+ * run frames until the camera has no fade left, or a limit
+ * @returns the number of frames run
+ */
+const runFade = () => {
+	let frames = 0;
+	while (app.viewport.getCameraEffect(FadeEffect) && frames < 400) {
+		tick();
+		frames++;
+	}
+	return frames;
+};
+
+describe("viewport.fadeIn() / fadeOut() call onComplete (#1810)", () => {
+	beforeAll(async () => {
+		boot();
+		app = new Application(320, 240, {
+			parent: "screen",
+			renderer: video.CANVAS,
+			consoleHeader: false,
+		});
+		await app.init();
+		now = globalThis.performance.now();
+	});
+
+	it("fadeIn calls onComplete once, then the fade is gone, at every duration", () => {
+		const missed = [];
+		for (let duration = 100; duration <= 1000; duration += 25) {
+			const onComplete = vi.fn();
+			app.viewport.fadeIn("#000000", duration, onComplete);
+			runFade();
+			expect(app.viewport.getCameraEffect(FadeEffect)).toBeUndefined();
+			if (onComplete.mock.calls.length !== 1) {
+				missed.push(duration);
+			}
+		}
+		expect(missed).toEqual([]);
+	});
+
+	it("fadeOut calls onComplete once, then the fade is gone, at every duration", () => {
+		const missed = [];
+		for (let duration = 100; duration <= 1000; duration += 25) {
+			const onComplete = vi.fn();
+			app.viewport.fadeOut("#000000", duration, onComplete);
+			runFade();
+			expect(app.viewport.getCameraEffect(FadeEffect)).toBeUndefined();
+			if (onComplete.mock.calls.length !== 1) {
+				missed.push(duration);
+			}
+		}
+		expect(missed).toEqual([]);
+	});
+
+	it("the issue's case: a scene change chained on fadeIn callbacks reaches its end", () => {
+		// as the Alex 4 outro moved from scene to scene in a fadeIn callback
+		const scenes = [];
+		const next = (n) => {
+			scenes.push(n);
+			if (n < 4) {
+				app.viewport.fadeIn("#000000", 300, () => {
+					next(n + 1);
+				});
+			}
+		};
+		next(1);
+		for (let i = 0; i < 4 && app.viewport.getCameraEffect(FadeEffect); i++) {
+			runFade();
+		}
+		expect(scenes).toEqual([1, 2, 3, 4]);
+	});
+
+	it("the fade ends on the frame after its tween does, not before", () => {
+		const onComplete = vi.fn();
+		app.viewport.fadeIn("#000000", 300, onComplete);
+		const fade = app.viewport.getCameraEffect(FadeEffect);
+		let frames = 0;
+		while (!onComplete.mock.calls.length && frames < 400) {
+			// still there for every frame before the callback
+			expect(app.viewport.getCameraEffect(FadeEffect)).toBe(fade);
+			tick();
+			frames++;
+		}
+		expect(onComplete).toHaveBeenCalledTimes(1);
+		// 300 ms is 18 frames at 60 fps
+		expect(frames).toBeGreaterThanOrEqual(18);
+		tick();
+		expect(app.viewport.getCameraEffect(FadeEffect)).toBeUndefined();
+	});
+});