From 30d60a43e80fb93c5cc372bbb580cd6cd30edfd4 Mon Sep 17 00:00:00 2001
From: cpojer <christoph.pojer@gmail.com>
Date: Wed, 30 Oct 2024 14:16:13 +0900
Subject: [PATCH] Fix an issue where the order of objectives affected whether
 an optional objective was triggered or not.

GitOrigin-RevId: 3f1c3e93ee329348d2f2239013e15d6302eb9a6f
---
 apollo/Objective.tsx                      |  1 -
 athena/Objectives.tsx                     | 10 ++-
 hera/editor/panels/EffectsPanel.tsx       | 87 +++++++++++------------
 hera/editor/panels/ObjectivePanel.tsx     | 69 +++++++++---------
 hera/editor/selectors/EffectSelector.tsx  |  9 ++-
 tests/__tests__/CustomObjectives.test.tsx | 66 ++++++++++++++++-
 6 files changed, 155 insertions(+), 87 deletions(-)

diff --git a/apollo/Objective.tsx b/apollo/Objective.tsx
index e1d08e38..30165177 100644
--- a/apollo/Objective.tsx
+++ b/apollo/Objective.tsx
@@ -136,7 +136,6 @@ export function applyObjectives(
 
     optionalObjective = null;
     reevaluate = true;
-    map = gameState.at(-1)![1];
 
     const [objectiveId, objective] =
       checkObjectives(previousMap, map, lastActionResponse) || [];
diff --git a/athena/Objectives.tsx b/athena/Objectives.tsx
index 5166d0ec..330c9b73 100644
--- a/athena/Objectives.tsx
+++ b/athena/Objectives.tsx
@@ -627,8 +627,11 @@ export function decodeObjective(objective: PlainObjective): Objective {
   }
 }
 
+const sortObjective = ([, objective]: [number, Objective]) =>
+  objective.type === Criteria.Default ? -1 : objective.optional ? 0 : 1;
+
 export function encodeObjectives(objectives: Objectives): PlainObjectives {
-  return sortBy([...objectives], ([id]) => id).map(([id, objective]) => [
+  return sortBy([...objectives], sortObjective).map(([id, objective]) => [
     id,
     encodeObjective(objective),
   ]);
@@ -636,7 +639,10 @@ export function encodeObjectives(objectives: Objectives): PlainObjectives {
 
 export function decodeObjectives(objectives: PlainObjectives) {
   return ImmutableMap(
-    objectives.map(([id, objective]) => [id, decodeObjective(objective)]),
+    sortBy(
+      objectives.map(([id, objective]) => [id, decodeObjective(objective)]),
+      sortObjective,
+    ),
   );
 }
 
diff --git a/hera/editor/panels/EffectsPanel.tsx b/hera/editor/panels/EffectsPanel.tsx
index 2db1bb2b..51700c70 100644
--- a/hera/editor/panels/EffectsPanel.tsx
+++ b/hera/editor/panels/EffectsPanel.tsx
@@ -4,6 +4,7 @@ import { Crystal } from '@deities/athena/invasions/Crystal.tsx';
 import MapData from '@deities/athena/MapData.tsx';
 import { Criteria } from '@deities/athena/Objectives.tsx';
 import isPresent from '@deities/hephaestus/isPresent.tsx';
+import sortBy from '@deities/hephaestus/sortBy.tsx';
 import UnknownTypeError from '@deities/hephaestus/UnknownTypeError.tsx';
 import Box from '@deities/ui/Box.tsx';
 import Button from '@deities/ui/Button.tsx';
@@ -92,59 +93,55 @@ export default function EffectsPanel({
   }, [effects]);
 
   const possibleEffects = useMemo(
-    () =>
-      [
-        ...(['win', 'lose', 'draw'] as const).map((id) =>
-          conditionsByID?.has(id) ? null : (
+    () => [
+      ...(['win', 'lose', 'draw'] as const).map((id) =>
+        conditionsByID?.has(id) ? null : (
+          <InlineLink
+            className={fitContentStyle}
+            key={id}
+            onClick={() => {
+              setShowNewEffects(false);
+              setEditorState(selectObjectiveEffect(editor, id));
+            }}
+          >
+            <EffectObjectiveTitle id={id} />
+          </InlineLink>
+        ),
+      ),
+      sortBy([...objectives], ([id]) => id)
+        .map(([id, condition]) => {
+          if (condition.type === Criteria.Default || conditionsByID?.has(id)) {
+            return null;
+          }
+
+          const type = condition.optional ? 'OptionalObjective' : 'GameEnd';
+          return (
             <InlineLink
               className={fitContentStyle}
               key={id}
               onClick={() => {
                 setShowNewEffects(false);
-                setEditorState(selectObjectiveEffect(editor, id));
+                setEditorState(selectObjectiveEffect(editor, id, condition));
               }}
             >
-              <EffectObjectiveTitle id={id} />
-            </InlineLink>
-          ),
-        ),
-        ...objectives
-          .map((condition, id) => {
-            if (
-              condition.type === Criteria.Default ||
-              conditionsByID?.has(id)
-            ) {
-              return null;
-            }
-
-            const type = condition.optional ? 'OptionalObjective' : 'GameEnd';
-            return (
-              <InlineLink
-                className={fitContentStyle}
-                key={id}
-                onClick={() => {
-                  setShowNewEffects(false);
-                  setEditorState(selectObjectiveEffect(editor, id, condition));
+              <EffectTitle
+                effect={{
+                  actions: [],
+                  conditions: [
+                    {
+                      type,
+                      value: id,
+                    },
+                  ],
                 }}
-              >
-                <EffectTitle
-                  effect={{
-                    actions: [],
-                    conditions: [
-                      {
-                        type,
-                        value: id,
-                      },
-                    ],
-                  }}
-                  objectives={objectives}
-                  trigger={type}
-                />
-              </InlineLink>
-            );
-          })
-          .values(),
-      ].filter(isPresent),
+                objectives={objectives}
+                trigger={type}
+              />
+            </InlineLink>
+          );
+        })
+        .filter(isPresent),
+    ],
     [conditionsByID, editor, setEditorState, objectives],
   );
 
diff --git a/hera/editor/panels/ObjectivePanel.tsx b/hera/editor/panels/ObjectivePanel.tsx
index 79d353c3..6e2db288 100644
--- a/hera/editor/panels/ObjectivePanel.tsx
+++ b/hera/editor/panels/ObjectivePanel.tsx
@@ -12,6 +12,7 @@ import {
   validateObjective,
 } from '@deities/athena/Objectives.tsx';
 import groupBy from '@deities/hephaestus/groupBy.tsx';
+import sortBy from '@deities/hephaestus/sortBy.tsx';
 import levelUsesObjective from '@deities/hermes/levelUsesObjective.tsx';
 import toLevelMap from '@deities/hermes/toLevelMap.tsx';
 import { ClientLevelID } from '@deities/hermes/Types.tsx';
@@ -251,43 +252,37 @@ export default function ObjectivePanel({
 
   return (
     <Stack className={paddingStyle} gap={24} vertical>
-      {[
-        ...objectives
-          .map((objective, id) => (
-            <ObjectiveCard
-              campaigns={
-                objectivesInCampaigns?.filter(
-                  ({ level }) => level && levelUsesObjective(id, level),
-                ) || null
-              }
-              canDelete={
-                objectives.size > 1 || objective.type !== Criteria.Default
-              }
-              canEditPerformance={canEditPerformance}
-              hasContentRestrictions={hasContentRestrictions}
-              id={id}
-              isAdmin={isAdmin}
-              key={id}
-              map={mapWithActivePlayers}
-              objective={objective}
-              onChange={(objective) => updateObjective(id, objective)}
-              selectEffect={() =>
-                setEditorState(selectObjectiveEffect(editor, id, objective))
-              }
-              selectLocation={() => {
-                if (objectiveHasVectors(objective)) {
-                  setEditorState({
-                    objective: { objective, objectiveId: id },
-                  });
-                }
-              }}
-              tags={tags}
-              user={user}
-              validate={validate}
-            />
-          ))
-          .values(),
-      ]}
+      {sortBy([...objectives], ([id]) => id).map(([id, objective]) => (
+        <ObjectiveCard
+          campaigns={
+            objectivesInCampaigns?.filter(
+              ({ level }) => level && levelUsesObjective(id, level),
+            ) || null
+          }
+          canDelete={objectives.size > 1 || objective.type !== Criteria.Default}
+          canEditPerformance={canEditPerformance}
+          hasContentRestrictions={hasContentRestrictions}
+          id={id}
+          isAdmin={isAdmin}
+          key={id}
+          map={mapWithActivePlayers}
+          objective={objective}
+          onChange={(objective) => updateObjective(id, objective)}
+          selectEffect={() =>
+            setEditorState(selectObjectiveEffect(editor, id, objective))
+          }
+          selectLocation={() => {
+            if (objectiveHasVectors(objective)) {
+              setEditorState({
+                objective: { objective, objectiveId: id },
+              });
+            }
+          }}
+          tags={tags}
+          user={user}
+          validate={validate}
+        />
+      ))}
       <Select
         selectedItem={
           <fbt desc="Headline for adding a new objective">New Objective</fbt>
diff --git a/hera/editor/selectors/EffectSelector.tsx b/hera/editor/selectors/EffectSelector.tsx
index 6dfa6ba2..25359df4 100644
--- a/hera/editor/selectors/EffectSelector.tsx
+++ b/hera/editor/selectors/EffectSelector.tsx
@@ -1,5 +1,6 @@
 import { Effects, Scenario } from '@deities/apollo/Effects.tsx';
 import { Objectives } from '@deities/athena/Objectives.tsx';
+import sortBy from '@deities/hephaestus/sortBy.tsx';
 import InlineLink from '@deities/ui/InlineLink.tsx';
 import Select from '@deities/ui/Select.tsx';
 import EffectTitle from '../lib/EffectTitle.tsx';
@@ -26,7 +27,13 @@ export default function EffectSelector({
         />
       }
     >
-      {[...effects].flatMap(([trigger, list]) =>
+      {sortBy([...effects], ([trigger]) =>
+        trigger === 'Start'
+          ? Number.NEGATIVE_INFINITY
+          : trigger === 'GameEnd'
+            ? Number.POSITIVE_INFINITY
+            : 0,
+      ).flatMap(([trigger, list]) =>
         [...list].map((effect, key) => (
           <InlineLink
             key={`${trigger}-${key}`}
diff --git a/tests/__tests__/CustomObjectives.test.tsx b/tests/__tests__/CustomObjectives.test.tsx
index f240b5bf..52c17a55 100644
--- a/tests/__tests__/CustomObjectives.test.tsx
+++ b/tests/__tests__/CustomObjectives.test.tsx
@@ -44,6 +44,8 @@ import vec from '@deities/athena/map/vec.tsx';
 import MapData, { SizeVector } from '@deities/athena/MapData.tsx';
 import {
   Criteria,
+  decodeObjectives,
+  encodeObjectives,
   Objective,
   validateObjectives,
 } from '@deities/athena/Objectives.tsx';
@@ -75,8 +77,13 @@ const player2 = HumanPlayer.from(map.getPlayer(2), '4');
 
 const defaultObjective = { hidden: false, type: Criteria.Default } as const;
 
+// Sort objectives by optional/non-optional.
 const defineObjectives = (objectives: ReadonlyArray<Objective>) =>
-  ImmutableMap(objectives.map((objective, index) => [index, objective]));
+  decodeObjectives(
+    encodeObjectives(
+      ImmutableMap(objectives.map((objective, index) => [index, objective])),
+    ),
+  );
 
 const optional = (map: MapData) =>
   map.copy({
@@ -3662,3 +3669,60 @@ test('rescuing a unit part of an objective of another player ends the game if th
     "Rescue (2,1 → 1,1) { player: 1, name: -13 }"
   `);
 });
+
+test('objectives are sorted when decoding so optional objectives are always triggered before non-optional ones', async () => {
+  const v1 = vec(1, 1);
+  const v2 = vec(1, 2);
+  const mapA = map.copy({
+    buildings: map.buildings.set(v1, House.create(player2, { label: 2 })),
+    config: map.config.copy({
+      objectives: ImmutableMap([
+        [
+          0,
+          {
+            hidden: false,
+            label: new Set([2]),
+            optional: false,
+            type: Criteria.DefeatLabel,
+          },
+        ],
+        [
+          1,
+          {
+            hidden: false,
+            label: new Set([2]),
+            optional: true,
+            type: Criteria.DefeatLabel,
+          },
+        ],
+      ]),
+    }),
+    units: map.units
+      .set(v1, Flamethrower.create(player1))
+      .set(v2, Pioneer.create(player2, { label: 2 })),
+  });
+
+  expect(validateObjectives(mapA)).toBe(true);
+
+  const [, gameActionResponseA] = await executeGameActions(mapA, [
+    AttackUnitAction(v1, v2),
+  ]);
+
+  expect(snapshotEncodedActionResponse(gameActionResponseA))
+    .toMatchInlineSnapshot(`
+    "AttackUnit (1,1 → 1,2) { hasCounterAttack: false, playerA: 1, playerB: 2, unitA: DryUnit { health: 100, ammo: [ [ 1, 3 ] ] }, unitB: null, chargeA: 33, chargeB: 100 }
+    GameEnd { objective: { bonus: undefined, completed: Set(0) {}, hidden: false, label: [ 2 ], optional: false, players: [], reward: null, type: 3 }, objectiveId: 0, toPlayer: 1, chaosStars: null }"
+  `);
+
+  const [, gameActionResponseB] = await executeGameActions(
+    MapData.fromJSON(JSON.stringify(mapA)),
+    [AttackUnitAction(v1, v2)],
+  );
+  expect(snapshotEncodedActionResponse(gameActionResponseB))
+    .toMatchInlineSnapshot(`
+      "AttackUnit (1,1 → 1,2) { hasCounterAttack: false, playerA: 1, playerB: 2, unitA: DryUnit { health: 100, ammo: [ [ 1, 3 ] ] }, unitB: null, chargeA: 33, chargeB: 100 }
+      OptionalObjective { objective: { bonus: undefined, completed: Set(1) { 1 }, hidden: false, label: [ 2 ], optional: true, players: [], reward: null, type: 3 }, objectiveId: 1, toPlayer: 1 }
+      AttackUnitGameOver { fromPlayer: 2, toPlayer: 1 }
+      GameEnd { objective: null, objectiveId: null, toPlayer: 1, chaosStars: null }"
+    `);
+});