Conversation
feat: added defeat animation for units and upgrades feat: added animation for shield breaks chore: prep for animations prefs feat: added healing animation
mariotee
requested review from
AMMayberry1,
CDMayberry,
Frugghi,
adbronico,
brianries,
danbastin and
moyerr
as code owners
mariotee
changed the title
| @@ -0,0 +1,323 @@ | |||
| import { AnimationType, type AnimationEvent } from './AnimationTypes'; | |||
|
|
|||
| // Animation duration constants (in milliseconds) | |||
There was a problem hiding this comment.
These feel like details that should be managed on the client side
| * Helper functions for constructing animation events. | ||
| * Provides static builder methods for common game actions. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-extraneous-class |
There was a problem hiding this comment.
I'd prefer that we conform to the TS recommended practice here and use a collection of exported functions instead of a static class
| /** | ||
| * Creates a damage animation event for a single target. | ||
| * | ||
| * @param targetId - Card UUID or base identifier (P1BASE/P2BASE) |
There was a problem hiding this comment.
Is there a reason that we need to use the special PXBASE code instead of just sending the uuid for the base?
| targetId: string, | ||
| amount: number, | ||
| sourceId?: string, | ||
| metadata?: Record<string, unknown> |
There was a problem hiding this comment.
We try very hard not to use untyped collections like these, even for configuration objects. Copilot should be able to declare explicit interface types really easily, so let's do that
| public static damage( | ||
| targetId: string, | ||
| amount: number, | ||
| sourceId?: string, |
There was a problem hiding this comment.
What's the point of sourceId? Looks like we aren't really using it for anything
| event.damageDealt = event.card.addDamage(eventDamageAmount, event.damageSource); | ||
|
|
||
| // Store animation data on the event to be processed in batch after the event window resolves | ||
| if (event.damageDealt > 0 && event.context?.game) { |
There was a problem hiding this comment.
We can remove the event.context?.game check, if that isn't present we have much bigger problems lol
| const isUpgrade = card.isUpgrade(); | ||
|
|
||
| // Check if this is a shield token being defeated | ||
| if (card.isTokenUpgrade() && card.internalName === 'shield' && card.parentCard) { |
There was a problem hiding this comment.
nit: we can do card.isShield() instead of checking internalName
| shieldUuid: card.uuid, // The specific shield token being removed | ||
| }; | ||
| } else { | ||
| // For units, include their upgrades so they can be animated |
There was a problem hiding this comment.
We shouldn't do this here, separate defeat events will be automatically generated for each upgrade
| const isUpgrade = card.isUpgrade(); | ||
|
|
||
| // Check if this is a shield token being defeated | ||
| if (card.isTokenUpgrade() && card.internalName === 'shield' && card.parentCard) { |
There was a problem hiding this comment.
Not sure where to put this but, the animation sequencing looks a bit weird when a unit with a shield is defeat via an effect like Takedown. The shield defeat happens first, then the unit defeat. Imo they should both happen at the same time.
| * Manages animation queue during game state updates on the server. | ||
| * Handles queuing, optimization, and serialization of animation events. | ||
| */ | ||
| export class ServerAnimationQueue { |
There was a problem hiding this comment.
We probably need to discuss how to reset this when an undo happens, probably just need to make sure it gets cleared out
2 participants
feat: added defeat animation for units and upgrades
feat: added animation for shield breaks
chore: prep for animations prefs
feat: added healing animation