Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #336: Mirror #340

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix issue #336: Mirror #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3158854
Fix issue #336: Mirror
openhands-agent Jan 28, 2025
3ba8b02
Fix pr #340: Fix issue #336: Mirror
openhands-agent Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions app/src/libs/combat/tags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1546,6 +1546,58 @@ export const summonPrevent = (
};

/** Prevent target from being stunned */
/** Mirror negative effects from caster to target */
export const mirror = (
effect: UserEffect,
usersEffects: UserEffect[],
target: BattleUserState,
) => {
if (!effect.isNew && !effect.castThisRound) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add validation for effect.userId.

The function assumes effect.userId exists when filtering negative effects, but it's not validated. This could lead to runtime errors if the effect doesn't have a userId.

if (!effect.isNew && !effect.castThisRound) {
+ if (!effect.userId) {
+   return {
+     txt: `Failed to mirror effects: Invalid effect userId`,
+     color: "red",
+   };
+ }
  // Find all negative effects on the caster

Committable suggestion skipped: line range outside the PR's diff.

// Find all negative effects on the caster
const negativeEffects = usersEffects.filter(
(e) => e.targetId === effect.creatorId && isNegativeUserEffect(e),
);

// Apply each negative effect to the target
negativeEffects.forEach((negEffect) => {
// Create a new effect with the same properties but different target
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
const mirroredEffect = {
type: negEffect.type,
id: nanoid(),
targetId: effect.targetId,
creatorId: effect.creatorId,
isNew: true,
castThisRound: true,
createdRound: effect.createdRound,
power: negEffect.power ?? 0,
powerPerLevel: negEffect.powerPerLevel ?? 0,
rounds: negEffect.rounds ?? 0,
direction: negEffect.direction ?? "offence",
calculation: negEffect.calculation ?? "static",
description: negEffect.description ?? "",
target: negEffect.target ?? "INHERIT",
friendlyFire: negEffect.friendlyFire,
staticAssetPath: negEffect.staticAssetPath ?? "",
staticAnimation: negEffect.staticAnimation ?? "",
appearAnimation: negEffect.appearAnimation ?? "",
disappearAnimation: negEffect.disappearAnimation ?? "",
timeTracker: negEffect.timeTracker,
} as const;
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
const newEffect = structuredClone(mirroredEffect) as unknown as UserEffect;
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
usersEffects.push(newEffect);
Comment on lines +1588 to +1590
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize memory usage and improve type safety.

The code uses structuredClone for deep cloning, which can be memory intensive for large objects. Additionally, the type assertions can be removed with proper typing.

- const newEffect = structuredClone(mirroredEffect) as unknown as UserEffect;
- usersEffects.push(newEffect);
+ const newEffect = { ...mirroredEffect } as UserEffect;
+ try {
+   usersEffects.push(newEffect);
+ } catch (error) {
+   console.error('Failed to add mirrored effect:', error);
+   return {
+     txt: `Failed to mirror effects: ${error.message}`,
+     color: "red",
+   };
+ }

Committable suggestion skipped: line range outside the PR's diff.

});
}

return getInfo(
target,
effect,
`all negative effects on ${target.username} are mirrored to their opponent`,
);
Comment on lines +1594 to +1598
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for edge cases.

The function should handle potential edge cases:

  1. Empty usersEffects array
  2. Invalid effect properties
  3. Failed effect application
 return getInfo(
   target,
   effect,
-  `all negative effects on ${target.username} are mirrored to their opponent`,
+  negativeEffects.length > 0
+    ? `all negative effects on ${target.username} are mirrored to their opponent`
+    : `no negative effects found to mirror`,
 );
📝 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
return getInfo(
target,
effect,
`all negative effects on ${target.username} are mirrored to their opponent`,
);
return getInfo(
target,
effect,
negativeEffects.length > 0
? `all negative effects on ${target.username} are mirrored to their opponent`
: `no negative effects found to mirror`,
);

};

export const weakness = (effect: UserEffect, target: BattleUserState) => {
const { power } = getPower(effect);
const mainCheck = Math.random() < power / 100;
Expand Down
9 changes: 9 additions & 0 deletions app/src/libs/combat/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,14 @@ export const DecreaseHealGivenTag = z.object({
calculation: z.enum(["static", "percentage"]).default("percentage"),
});

export const MirrorTag = z.object({
...BaseAttributes,
type: z.literal("mirror").default("mirror"),
description: msg("Applies all negative effects from the caster to the target"),
target: z.enum(BaseTagTargets).optional().default("INHERIT"),
direction: type("defence"),
});

export const IncreasePoolCostTag = z.object({
...BaseAttributes,
...PowerAttributes,
Expand Down Expand Up @@ -675,6 +683,7 @@ export const IncreaseMarriageSlots = z.object({
/** UNIONS OF TAGS **/
/******************** */
export const AllTags = z.union([
MirrorTag.default({}),
AbsorbTag.default({}),
BarrierTag.default({}),
BuffPreventTag.default({}),
Expand Down