Store animated sprites as dynamic resources#3408
Draft
DevelopingTom wants to merge 2 commits intoopenfrontio:mainfrom
Draft
Store animated sprites as dynamic resources#3408DevelopingTom wants to merge 2 commits intoopenfrontio:mainfrom
DevelopingTom wants to merge 2 commits intoopenfrontio:mainfrom
Conversation
DevelopingTom
added
the
Refactor
Contributor
WalkthroughAdds a JSON animated-sprite config and refactors AnimatedSprite to accept an AnimatedSpriteConfig. AnimatedSpriteLoader validates the JSON (zod), loads images from /animatedSprites/{name}.png, creates/caches AnimatedSpriteCanvas objects, and provides colorized AnimatedSprite instances built from cached canvases. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI/Client
participant Loader as AnimatedSpriteLoader
participant Server as File Server
participant Factory as CanvasFactory
participant Sprite as AnimatedSprite
UI->>Loader: loadAllAnimatedSpriteImages()
Loader->>Server: GET /animatedSprites/config.json
Server-->>Loader: config.json
Loader->>Loader: validate config (zod)
loop for each sprite
Loader->>Server: GET /animatedSprites/{name}.png
Server-->>Loader: image data
Loader->>Factory: createCanvas(image data)
Factory-->>Loader: AnimatedSpriteCanvas
Loader->>Loader: cache AnimatedSpriteCanvas by name
end
UI->>Loader: request colored sprite (owner, name, theme)
Loader->>Loader: get cached AnimatedSpriteCanvas
Loader->>Factory: createColoredAnimatedSpriteCanvas(canvas, theme)
Factory-->>Loader: colorized AnimatedSpriteCanvas
Loader->>Sprite: wrap canvas into AnimatedSprite(config)
Sprite-->>UI: AnimatedSprite instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/client/graphics/AnimatedSpriteLoader.ts (1)
68-71: Type mismatch:FxTypeenum vs string map keys.The method signature takes
FxType, butanimatedSpriteImageMapkeys areconfig.namestrings from the JSON. This works becauseFxTypevalues are string literals, but it creates a hidden coupling: ifFxType.MiniBigSmoke = "MiniBigSmoke"but config.json has"BigSmoke", the lookup fails silently.Consider adding a type assertion or explicit mapping to make this coupling visible and safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/AnimatedSpriteLoader.ts` around lines 68 - 71, The lookup in createRegularAnimatedSprite uses FxType to index animatedSpriteImageMap which actually uses config.name string keys, creating a fragile hidden coupling; update createRegularAnimatedSprite to explicitly map or coerce FxType to the expected config name (e.g., use a mapping object or a conversion function) before calling animatedSpriteImageMap.get, validate the mapped key exists and handle/memoize missing keys deterministically (returning null or fallback), and add a comment documenting the mapping between FxType values and config.name keys to make the dependency explicit; refer to the createRegularAnimatedSprite method, the animatedSpriteImageMap collection, and the FxType enum when implementing the mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/animatedSprites/config.json`:
- Around line 19-26: The sprite config uses "BigSmoke" but the FxType enum and
NukeFx lookup expect "MiniBigSmoke", so rename the "name" field in the JSON
object from "BigSmoke" to "MiniBigSmoke" in
resources/animatedSprites/config.json so the loader can find the sprite
referenced by FxType::MiniBigSmoke and the NukeFx (referenced in NukeFx.ts) will
render correctly.
In `@src/client/graphics/AnimatedSprite.ts`:
- Around line 87-90: The setOrigin method mutates the shared this.config
(originX/originY) which is supplied by AnimatedSpriteLoader via
animatedSpriteImageMap; change to store origin on the instance instead: add
instance fields (e.g. this.originX, this.originY) initialized from this.config
in the AnimatedSprite constructor, update setOrigin to assign these instance
fields (not this.config), and update any internal uses that read
this.config.originX/ originY to use the instance fields so changing one sprite’s
origin won’t affect others.
In `@src/client/graphics/AnimatedSpriteLoader.ts`:
- Around line 33-55: In loadAllAnimatedSpriteImages, stop using result.data!
when safeParse can fail: after calling
AnimatedSpriteConfigsSchema.safeParse(animationConfig) check result.success and
if false either throw the validation error (replacing safeParse with parse) or
return early after logging the error so you don't proceed to
result.data!.animatedSprites; update the function (references:
AnimatedSpriteConfigsSchema.safeParse / .parse, loadAllAnimatedSpriteImages,
result.data!.animatedSprites, createCanvas) to ensure you only run Promise.all
when validation succeeded.
- Around line 73-95: The cache key in createColoredAnimatedSpriteForUnit omits
the theme, so coloredAnimatedSpriteCache can return sprites with wrong colors
when theme varies; change the key construction (the local key variable used in
coloredAnimatedSpriteCache.get/set) to include a unique theme identifier (for
example theme.name, theme.id, or theme.spawnHighlightColor()) so the lookup and
subsequent set use the theme-aware key, leaving the rest of
createColoredAnimatedSpriteForUnit and the call to
createColoredAnimatedSpriteCanvas unchanged.
---
Nitpick comments:
In `@src/client/graphics/AnimatedSpriteLoader.ts`:
- Around line 68-71: The lookup in createRegularAnimatedSprite uses FxType to
index animatedSpriteImageMap which actually uses config.name string keys,
creating a fragile hidden coupling; update createRegularAnimatedSprite to
explicitly map or coerce FxType to the expected config name (e.g., use a mapping
object or a conversion function) before calling animatedSpriteImageMap.get,
validate the mapped key exists and handle/memoize missing keys deterministically
(returning null or fallback), and add a comment documenting the mapping between
FxType values and config.name keys to make the dependency explicit; refer to the
createRegularAnimatedSprite method, the animatedSpriteImageMap collection, and
the FxType enum when implementing the mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b8b42b4-b259-4c1f-8014-b8b5a282468a
⛔ Files ignored due to path filters (12)
resources/animatedSprites/BigSmoke.pngis excluded by!**/*.pngresources/animatedSprites/BuildingExplosion.pngis excluded by!**/*.pngresources/animatedSprites/Conquest.pngis excluded by!**/*.pngresources/animatedSprites/Dust.pngis excluded by!**/*.pngresources/animatedSprites/MiniExplosion.pngis excluded by!**/*.pngresources/animatedSprites/MiniFire.pngis excluded by!**/*.pngresources/animatedSprites/MiniSmoke.pngis excluded by!**/*.pngresources/animatedSprites/MiniSmokeAndFire.pngis excluded by!**/*.pngresources/animatedSprites/Nuke.pngis excluded by!**/*.pngresources/animatedSprites/SAMExplosion.pngis excluded by!**/*.pngresources/animatedSprites/SinkingShip.pngis excluded by!**/*.pngresources/animatedSprites/UnitExplosion.pngis excluded by!**/*.png
📒 Files selected for processing (3)
resources/animatedSprites/config.jsonsrc/client/graphics/AnimatedSprite.tssrc/client/graphics/AnimatedSpriteLoader.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/client/graphics/AnimatedSpriteLoader.ts (1)
75-113:⚠️ Potential issue | 🟡 MinorInclude the theme in the colored-sprite cache key.
The cached canvas depends on
theme.spawnHighlightColor(), but the key only usesfxTypeandowner.id(). If the same loader is reused after a theme change, it can return the wrong cached colors.💡 Proposed fix
- const key = `${fxType}-${owner.id()}`; + const key = `${fxType}-${owner.id()}-${theme.spawnHighlightColor()}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/AnimatedSpriteLoader.ts` around lines 75 - 113, The cache key for colored sprites in createColoredAnimatedSpriteForUnit only uses `${fxType}-${owner.id()}` but the result also depends on theme.spawnHighlightColor(); update the key generation used with coloredAnimatedSpriteCache to include the theme (e.g., incorporate theme.spawnHighlightColor() or another unique theme identifier) so that createColoredAnimatedSpriteForUnit and its use of createColoredAnimatedSpriteCanvas return a cached sprite only when fxType, owner.id() and theme match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/AnimatedSprite.ts`:
- Around line 36-50: The update method in AnimatedSprite currently advances at
most one frame per call; change it to consume the entire deltaTime by looping:
accumulate elapsedTime, then while elapsedTime >= this.config.frameDuration
subtract frameDuration and advance this.currentFrame repeatedly until
elapsedTime < frameDuration, applying the same end-of-animation logic (handle
this.config.looping to wrap to 0 or set this.currentFrame =
this.config.frameCount - 1 and this.active = false) so multiple overdue frames
are processed in a single update call.
In `@src/client/graphics/AnimatedSpriteLoader.ts`:
- Around line 41-54: The image event handlers are attached after setting img.src
which can miss cached load/error events; in the animatedSprites mapping in
AnimatedSpriteLoader.ts, create the Promise and assign img.onload and
img.onerror (calling this.createCanvas(config, img) and resolving/rejecting)
before setting img.src to "/animatedSprites/" + config.name + ".png", ensuring
handlers are in place; keep the existing createCanvas(config, img) call and
error logging in the onerror handler.
---
Duplicate comments:
In `@src/client/graphics/AnimatedSpriteLoader.ts`:
- Around line 75-113: The cache key for colored sprites in
createColoredAnimatedSpriteForUnit only uses `${fxType}-${owner.id()}` but the
result also depends on theme.spawnHighlightColor(); update the key generation
used with coloredAnimatedSpriteCache to include the theme (e.g., incorporate
theme.spawnHighlightColor() or another unique theme identifier) so that
createColoredAnimatedSpriteForUnit and its use of
createColoredAnimatedSpriteCanvas return a cached sprite only when fxType,
owner.id() and theme match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8ccd0fa-26ed-47d2-830a-63e2bb2dbf03
⛔ Files ignored due to path filters (1)
resources/animatedSprites/MiniBigSmoke.pngis excluded by!**/*.png
📒 Files selected for processing (3)
resources/animatedSprites/config.jsonsrc/client/graphics/AnimatedSprite.tssrc/client/graphics/AnimatedSpriteLoader.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/animatedSprites/config.json
Comment on lines
36
to
50
| update(deltaTime: number) { | ||
| if (!this.active) return; | ||
| this.elapsedTime += deltaTime; | ||
| if (this.elapsedTime >= this.frameDuration) { | ||
| this.elapsedTime -= this.frameDuration; | ||
| if (this.elapsedTime >= this.config.frameDuration) { | ||
| this.elapsedTime -= this.config.frameDuration; | ||
| this.currentFrame++; | ||
|
|
||
| if (this.currentFrame >= this.frameCount) { | ||
| if (this.looping) { | ||
| if (this.currentFrame >= this.config.frameCount) { | ||
| if (this.config.looping) { | ||
| this.currentFrame = 0; | ||
| } else { | ||
| this.currentFrame = this.frameCount - 1; | ||
| this.currentFrame = this.config.frameCount - 1; | ||
| this.active = false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Consume the full deltaTime in update().
A long frame only advances one animation frame here. SpriteFx.renderTick() stops FX by wall-clock duration, so after a hitch this sprite can be removed before it catches up visually. Step through all overdue frames, not just one.
💡 Proposed fix
update(deltaTime: number) {
if (!this.active) return;
this.elapsedTime += deltaTime;
- if (this.elapsedTime >= this.config.frameDuration) {
+ while (this.active && this.elapsedTime >= this.config.frameDuration) {
this.elapsedTime -= this.config.frameDuration;
this.currentFrame++;
if (this.currentFrame >= this.config.frameCount) {
if (this.config.looping) {
this.currentFrame = 0;
} else {
this.currentFrame = this.config.frameCount - 1;
this.active = false;
}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| update(deltaTime: number) { | |
| if (!this.active) return; | |
| this.elapsedTime += deltaTime; | |
| if (this.elapsedTime >= this.frameDuration) { | |
| this.elapsedTime -= this.frameDuration; | |
| if (this.elapsedTime >= this.config.frameDuration) { | |
| this.elapsedTime -= this.config.frameDuration; | |
| this.currentFrame++; | |
| if (this.currentFrame >= this.frameCount) { | |
| if (this.looping) { | |
| if (this.currentFrame >= this.config.frameCount) { | |
| if (this.config.looping) { | |
| this.currentFrame = 0; | |
| } else { | |
| this.currentFrame = this.frameCount - 1; | |
| this.currentFrame = this.config.frameCount - 1; | |
| this.active = false; | |
| } | |
| } | |
| update(deltaTime: number) { | |
| if (!this.active) return; | |
| this.elapsedTime += deltaTime; | |
| while (this.active && this.elapsedTime >= this.config.frameDuration) { | |
| this.elapsedTime -= this.config.frameDuration; | |
| this.currentFrame++; | |
| if (this.currentFrame >= this.config.frameCount) { | |
| if (this.config.looping) { | |
| this.currentFrame = 0; | |
| } else { | |
| this.currentFrame = this.config.frameCount - 1; | |
| this.active = false; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/AnimatedSprite.ts` around lines 36 - 50, The update
method in AnimatedSprite currently advances at most one frame per call; change
it to consume the entire deltaTime by looping: accumulate elapsedTime, then
while elapsedTime >= this.config.frameDuration subtract frameDuration and
advance this.currentFrame repeatedly until elapsedTime < frameDuration, applying
the same end-of-animation logic (handle this.config.looping to wrap to 0 or set
this.currentFrame = this.config.frameCount - 1 and this.active = false) so
multiple overdue frames are processed in a single update call.
Comment on lines
+41
to
+54
| result.data!.animatedSprites.map((config) => { | ||
| const img = new Image(); | ||
| img.crossOrigin = "anonymous"; | ||
| img.src = "/animatedSprites/" + config.name + ".png"; | ||
| return new Promise<void>((resolve, reject) => { | ||
| img.onload = () => { | ||
| this.createCanvas(config, img); | ||
| resolve(); | ||
| }; | ||
| img.onerror = (e) => { | ||
| reject(e); | ||
| console.error(`Could not load animated sprite: `, e); | ||
| }; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In browsers, can new Image()miss theloadorerrorevent ifsrcis assigned beforeonloadandonerror, especially when the image is already cached?
💡 Result:
Yes—if you set img.src first and only later assign img.onload / img.onerror (or add listeners), you can miss the event, and this is especially likely when the image resolves “immediately” (e.g., from cache), because the load/error may fire before your handler is attached. This is a well-known gotcha; older/real-world browser behavior and common guidance is to attach handlers before setting src. [1], [2]
Robust patterns:
- Attach handlers first, then set
src: (avoids the race) [2] - Or, after setting
src, also checkimg.completeand (commonly)img.naturalWidth > 0to distinguish success vs failure;completebecomestruewhen the browser is done trying (success or failure). [1] - Or use
img.decode()andawait/.then(): it returns a promise that fulfills when decoding is complete (and will reject if it can’t be decoded). [3]
Example (safe order):
const img = new Image();
img.onload = () => console.log("loaded");
img.onerror = () => console.log("error");
img.src = url;Example (cover the cached/instant case):
const img = new Image();
img.onload = done;
img.onerror = fail;
img.src = url;
if (img.complete) (img.naturalWidth ? done() : fail());Sources: MDN on complete semantics [1]; guidance noting cached images can finish before handlers are bound [2]; HTML spec on img.decode() promise behavior [3].
Attach event handlers before setting image source.
When you set img.src, the browser starts loading immediately. If the image is cached, the load or error event fires before your handlers are attached, leaving the promise pending indefinitely and blocking the function.
💡 Proposed fix
result.data!.animatedSprites.map((config) => {
const img = new Image();
img.crossOrigin = "anonymous";
- img.src = "/animatedSprites/" + config.name + ".png";
return new Promise<void>((resolve, reject) => {
img.onload = () => {
this.createCanvas(config, img);
resolve();
};
img.onerror = (e) => {
console.error(`Could not load animated sprite: `, e);
reject(e);
};
+ img.src = `/animatedSprites/${config.name}.png`;
});
}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/AnimatedSpriteLoader.ts` around lines 41 - 54, The image
event handlers are attached after setting img.src which can miss cached
load/error events; in the animatedSprites mapping in AnimatedSpriteLoader.ts,
create the Promise and assign img.onload and img.onerror (calling
this.createCanvas(config, img) and resolving/rejecting) before setting img.src
to "/animatedSprites/" + config.name + ".png", ensuring handlers are in place;
keep the existing createCanvas(config, img) call and error logging in the
onerror handler.
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
1 participant
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.
Description:
Animated sprites are currently hardcoded, which prevents them from being loaded dynamically.
Changes:
The end-goal is to have per-player cosmetics, but this change should not have any visual impacts.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
IngloriousTom