loathers / grimoire

Apache License 2.0
3 stars 8 forks source link

don't rely on floating point equality for bonuses #89

Closed horrible-little-slime closed 1 year ago

horrible-little-slime commented 1 year ago

Ahh, you're right, that was the issue! I'll amend it in the morning, good shout

On Sat, May 6, 2023, 12:18 AM Jeffrey Dudek @.***> wrote:

@.**** commented on this pull request.

In src/outfit.ts https://github.com/Loathing-Associates-Scripting-Society/grimoire/pull/89#discussion_r1186615160 :

@@ -338,7 +338,8 @@ export class Outfit { } if (spec.bonuses) { for (const [item, value] of spec.bonuses) {

  • succeeded &&= value + this.getBonus(item) === this.addBonus(item, value);
  • this.addBonus(item, value);
  • succeeded &&= this.bonuses.has(item);

Right now, this will still fail if the outfit cannot possibly equip the bonus item, because .addBonus calls .setBonus which calls .canEquip. Is this what we want to happen?

It might be smoother to remove the .canEquip call from .setBonus. Then the .addBonus/.setBonus call can never fail, and we rely on the maximizer to ignore impossible bonus items.

— Reply to this email directly, view it on GitHub https://github.com/Loathing-Associates-Scripting-Society/grimoire/pull/89#pullrequestreview-1415665179, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASZNQVLQVS3NMLJ7XDC5RQLXEXGHRANCNFSM6AAAAAAXXPRTNA . You are receiving this because you authored the thread.Message ID: <Loathing-Associates-Scripting-Society/grimoire/pull/89/review/1415665179@ github.com>

horrible-little-slime commented 1 year ago

The original intent was to shrink maximizer strings; I'll look to see if that logic can be relocated

On Sat, May 6, 2023, 12:18 AM no no @.***> wrote:

Ahh, you're right, that was the issue! I'll amend it in the morning, good shout

On Sat, May 6, 2023, 12:18 AM Jeffrey Dudek @.***> wrote:

@.**** commented on this pull request.

In src/outfit.ts https://github.com/Loathing-Associates-Scripting-Society/grimoire/pull/89#discussion_r1186615160 :

@@ -338,7 +338,8 @@ export class Outfit { } if (spec.bonuses) { for (const [item, value] of spec.bonuses) {

  • succeeded &&= value + this.getBonus(item) === this.addBonus(item, value);
  • this.addBonus(item, value);
  • succeeded &&= this.bonuses.has(item);

Right now, this will still fail if the outfit cannot possibly equip the bonus item, because .addBonus calls .setBonus which calls .canEquip. Is this what we want to happen?

It might be smoother to remove the .canEquip call from .setBonus. Then the .addBonus/.setBonus call can never fail, and we rely on the maximizer to ignore impossible bonus items.

— Reply to this email directly, view it on GitHub https://github.com/Loathing-Associates-Scripting-Society/grimoire/pull/89#pullrequestreview-1415665179, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASZNQVLQVS3NMLJ7XDC5RQLXEXGHRANCNFSM6AAAAAAXXPRTNA . You are receiving this because you authored the thread.Message ID: <Loathing-Associates-Scripting-Society/grimoire/pull/89/review/1415665179 @github.com>