Fix add() method firing callbacks before animation completes - #21
Open
hughescr wants to merge 5 commits into
Open
Fix add() method firing callbacks before animation completes#21hughescr wants to merge 5 commits into
hughescr wants to merge 5 commits into
Conversation
rollMulti(specs): place multiple dice with per-die colorsets in a single physics throw; each returned set is tagged with its originating colorset so callers can attribute results per die. roll()/add()/reroll()/remove() behavior is unchanged (their results gain colorset:null/label:null fields). create(type, colordata): apply an optional per-die colorset override before baking the material. Environment reflections: stop hardcoding envMapIntensity=0 on PBR dice materials (respect each material type's configured intensity), and set a soft default scene.environment (RoomEnvironment via PMREMGenerator) so metallic and glossy dice render with reflections instead of appearing dark. Claude-Session: https://claude.ai/code/session_01S953CfDEvtvxEZomKTJBWS
…rset redesign Migrate sound playback from HTMLAudioElement to the Web Audio API (AudioContext/AudioBufferSourceNode), fixing animation stutter on iOS and desktop when sounds are enabled. AudioContext is created lazily and resumed from user-gesture call stacks so playback isn't silently blocked by autoplay/gesture restrictions. Physics performance: switch to CANNON.SAPBroadphase for collision broadphase, cap the per-frame catch-up loop so a stalled/backgrounded tab can't spiral into running many world.step() calls back-to-back, and make the pre-throw settle simulation yield to the event loop periodically instead of blocking the main thread. Correctness fixes: updateConfig() now applies merged options via Object.assign and only touches theme fields the caller actually passed (via property-presence checks) instead of wiping theme_customColorset back to null; rollMulti() claims ownership synchronously before its prewarm await to fix a supersession race; destroy() mid-add() now rejects add()'s Promise instead of silently resolving it; per-set +/- operators in dice notation (e.g. 2d6-1d4) are respected; signed modifier merging nets out correctly including exact cancellation to zero; forced/predetermined d4 results now produce the correct face; disotope's explicit per-face values are no longer collapsed into a min/max/step range; malformed forced-result (@) suffixes are rejected as malformed notation instead of silently dropping tokens. Malformed/unparseable notation now rejects the returned Promise with a descriptive Error, and a roll superseded by a newer roll()/rollMulti()/reroll()/add() call now rejects instead of hanging forever. Memory/lifecycle: add a destroy() method to fully tear down a DiceBox instance (stops in-flight rolls/animation, removes the resize listener, settles pending Promises, disposes GPU resources, closes the AudioContext, removes the canvas), add an LRU cache with deferred disposal for composite material textures, and cache/dispose bump normal-map textures per- DiceFactory instance rather than on a shared static registry. Colorsets redesigned for WCAG AAA text legibility (numeral/outline contrast against each die's body color), with several damage-type colorsets conceptually rethemed; colorset names and the theme_colorset selection API are unchanged. Packaging fixes: corrected the npm "files" allowlist, marked the package "type": "module" with a proper "exports" map (ESM-only, no CommonJS build), and removed .npmignore now that "files" governs the published tarball. Expanded README and added this CHANGELOG.md. Added src/index.html + src/demo.js, a dev demo page exercising roll/add/reroll/rollMulti/colorset switching for local development and manual testing. Claude-Session: https://claude.ai/code/session_018pN5dERVrqvBKQXFsoPYzv
The coin (d2) die's face labels and bump maps reference public/textures/silvercoin/, which was missing from the repo entirely (lost upstream; restored from the ancestor MajorVictory/3DDiceRoller project) — coin faces rendered blank without them. Also rename the dc preset's data key setBumpMaps -> bumpMaps so DicePreset's constructor actually loads the bump maps; the key was named after the setter method instead of the property it checks. Visual effect of the bump maps is subtle (the procedural label bump and metal composite dominate), but the preset data is now functional instead of dead. Claude-Session: https://claude.ai/code/session_018pN5dERVrqvBKQXFsoPYzv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
add()method firing completion handlers before dice animation completes.Problem
When calling
add()to add dice to an existing roll, the completion handlers (onAddDiceCompletecallback,addDiceCompleteevent, and Promise resolution) fire immediately on the first animation frame instead of waiting for the dice to settle.Root Cause
In the
add()method:simulateThrow()pre-simulates the physics to completion, leaving newly added dice inSLEEPINGstateanimateThrow()starts with the completion callbackthrowFinished()checks if all dice havesleepState >= SLEEPING— they do (from pre-simulation) — so the callback fires immediatelyWhy
roll()works correctlyThe
roll()method callsclearDice()first, which removes all existing dice. Fresh dice start in an active state and need time to settle, sothrowFinished()correctly waits.The Fix
After spawning dice and before starting animation, wake up the newly added dice so they need time to settle during the visible animation:
This ensures throwFinished() won't return true until the new dice actually settle.