feat: add undo-stack on a per map basis #22

tjamesmac · 2024-05-17T15:05:08Z

Description

Hoping to resolve #3
Adds the ability to:

save the undo stack on reload for unsaved maps
store undo stacks for saved maps

Todo

undo stack on reload one off (need to push)
move effects into undostack

Would appreciate any feedback and whether this has hit the mark! :)

cpojer · 2024-05-17T22:30:55Z

Here are the replies to your questions from the issue:

The MapObject.id is returning undefined and never returns an actual id. Is this expected? If it is, do you have any suggestions for generating a decent fallback?

If you rebase, now it will be an empty string when you "save" it.

Instead of restoring the previous map with the Restore button, i'm restoring if a previousState is present. When clicking New map I then delete the related localstorage items so a refresh doesn't continually bring back the last map. Does this sound like the right path for the U? Happy to change if I misinterpreted the requirement.

I think that makes the most sense, yes. If you don't hit restore before clicking "new map", we should wipe the undo state.

Also not sure if i'm missing something obvious but the confirmation dialogs for the new/reset map don't seem to work. I've just started calling new map directly

Yes that's fine. The docs page doesn't have those confirmation dialogs but they are in the game.

Lastly, are there tests for the map editor?

There are no tests 🪦 If you write a function (not React) that you want to test, feel free to add a test. Otherwise we are living in a brave world here.

cpojer · 2024-05-19T05:00:08Z

hera/editor/lib/updateUndoStack.tsx

+export const UNDO_STACK_KEY = (id: string | undefined) =>
+  `map-editor-undo-stack-${id ? `${id}` : 'fallback'}`;
+
+export const UNDO_STACK_INDEX_KEY = (id: string | undefined) =>
+  `map-editor-undo-stack-index-${id ? `${id}` : 'fallback'}`;
+


Since these are functions they should be getUndoStackKey and getUndoStackIndexKey.

cpojer · 2024-05-19T05:01:39Z

hera/editor/Types.tsx

 ) => void;

 type MapSaveType = 'New' | 'Update' | 'Disk' | 'Export';
 export type MapCreateVariables = Readonly<{
  effects: Effects;
+  id: string;


Let's remove this.

When you rebase, you'll be able to find this: https://github.com/nkzw-tech/athena-crisis/blob/main/docs/content/examples/map-editor.tsx#L93

Let's change that line to set the id to the same as slug, that means you'll always have a unique id to work with.

cpojer · 2024-05-19T05:02:41Z

hera/editor/MapEditor.tsx

+export function decodeUndoStack(
+  encodedUndoStack: string | null,
+): UndoStack | undefined {
+  if (!encodedUndoStack) {
+    return undefined;
+  }
+
+  try {
+    return JSON.parse(encodedUndoStack).map(
+      ([key, value]: [string, PlainMap]) => [key, MapData.fromObject(value)],
+    );
+  } catch {
+    return undefined;
+  }
+}


Why return undefined instead of null?

cpojer · 2024-05-19T05:03:33Z

hera/editor/MapEditor.tsx

+        }
+
+        const undoStackIndex = getUndoStackIndex(mapObject?.id);
+        const map = undoStack[undoStackIndex ?? -1][1];


What if this ends up being -1? It will crash, right?

cpojer · 2024-05-19T05:04:54Z

Hey @tjamesmac, thank you so much for your contributions! I left a few comments, would you mind taking a look?

if the map isn't saved, the undo stack is extended across reloads. Meaning, you can go back to the last refreshed map with ctrl-z.

I think this is ok actually, that's the whole point of replacing the "restore" feature with this one, as long as the id matches.

- change stack keys - remove id from MapCreateVariables type - use null instead of undefined - use array.length - 1 to calc last index

tjamesmac · 2024-05-20T10:49:03Z

Hey @cpojer, thanks for the quick review and sorry for the slowish response. Busy weekend!

I've made your suggestions and added a few more fixes for some edge cases. Would appreciate another review!

Also thanks for open sourcing Athena crisis :)

tjamesmac · 2024-05-20T10:51:03Z

hera/editor/MapEditor.tsx

@@ -212,7 +261,8 @@ export default function MapEditor({
    (size = new SizeVector(random(10, 15), random(10, 15))): MapData => {
      return withHumanPlayer(
        mapObject
-          ? MapData.fromJSON(mapObject.state)!
+          ? getInitialMapFromUndoStack(mapObject?.id)?.map ??


this restores the state from the correct position in the undostack when loading the map from the url

tjamesmac · 2024-05-20T10:52:11Z

hera/editor/MapEditor.tsx

+  useEffect(() => {
+    if (editor.undoStack) {
+      const stack = editor.undoStack.map(([key, value]) => [
+        key,
+        value.toJSON(),
+      ]);
+      Storage.setItem(getUndoStackKey(mapObject?.id), JSON.stringify(stack));
+    }
+  }, [editor.undoStack, mapObject?.id]);
+


moved the stack saving to localstorage from updateUndoStack to here because it was out of sync by one

cpojer · 2024-05-20T17:11:16Z

docs/content/examples/map-editor.tsx

@@ -80,6 +80,8 @@ export default function MapEditorExample() {

  const handleMapUpdate = useCallback(
    (variables: MapCreateVariables | MapUpdateVariables) => {
+      const nameToSlug = toSlug(variables.mapName);


Suggested change

const nameToSlug = toSlug(variables.mapName);

const slug = toSlug(variables.mapName);

cpojer · 2024-05-20T17:11:24Z

docs/content/examples/map-editor.tsx

@@ -90,9 +92,9 @@ export default function MapEditorExample() {
          username: viewer.username,
        },
        effects: JSON.stringify(encodeEffects(variables.effects)),
-        id: 'id' in variables ? variables.id : '',
+        id: 'id' in variables ? variables.id : nameToSlug,


Suggested change

id: 'id' in variables ? variables.id : nameToSlug,

id: 'id' in variables ? variables.id : slug,

cpojer · 2024-05-20T17:11:30Z

docs/content/examples/map-editor.tsx

        name: variables.mapName,
-        slug: toSlug(variables.mapName),
+        slug: nameToSlug,


Suggested change

slug: nameToSlug,

slug,

cpojer · 2024-05-20T17:14:41Z

hera/editor/lib/updateUndoStack.tsx

+export const getUndoStackKey = (id: string | undefined) =>
+  `map-editor-undo-stack-${id ? `${id}` : 'fallback'}`;
+
+export const getUndoStackIndexKey = (id: string | undefined) =>
+  `map-editor-undo-stack-index-${id ? `${id}` : 'fallback'}`;
+


Just a suggestion, curious what you think: From reading the code it's not obvious why the id is passed. What do you think about calling these getUndoStackKeyFor(id) and getUndoStackIndexKeyFor(id)?

Sorry, didn't come to me earlier since they were uppercased and that confused me 😅

Hah, not a problem at all. Definitely think adding the For makes the intent a little clearer

cpojer · 2024-05-20T17:18:41Z

hera/editor/MapEditor.tsx

@@ -319,40 +376,56 @@ export default function MapEditor({
  const [previousState, setPreviousState] =
    useState<PreviousMapEditorState | null>(() => {
      try {
+        const effects = Storage.getItem(EFFECTS_KEY) || '';
+


Let's remove this one empty line. There are too many empty lines here :D

cpojer · 2024-05-20T17:23:26Z

This is looking really solid! I think there are only two more considerations before it's ready:

When I edit a map, then reload and hit cmd+z, it restores the map with one state less than expected. Ideally it restores it at the same state as before.
Now that we are restoring maps with this approach, I think we need to also move effects into the undo stack. Effects are character messages etc. that you can define in the effects panel. Ideally they get versioned with the current state of the map as well. This should allow cleaning up a bit of the code that handles storage as we won't have to make use of a separate storage item for effects.

Just note, we should probably not immediately update undo every time an effect changes since a user can type into the textarea to update the text a character says and it would probably be slow and not fun to add each character to the undo stack.

tjamesmac · 2024-05-22T08:21:58Z

Hey thanks, got a bit busier than I expected. Will look at shifting the effects into the stack when I get chance

- nameToSlug -> slug - getUndoStackKey -> getUndoStackKeyFor - getUndoStackKeyIndex -> getUndoStackKeyIndexFor - remove empty line

tjamesmac · 2024-05-22T20:11:38Z

Hey @cpojer , I have a couple of questions!

When I edit a map, then reload and hit cmd+z, it restores the map with one state less than expected. Ideally it restores it at the same state as before.

How do you feel about the previous state (if present) just being restored straight off a reload instead of hitting restore to get back to the previous state? If that's okay I think I can check when getting the initial map state whether I should restore from:

undostack
mapobject
generate a new map

Ideally they get versioned with the current state of the map as well.

Does this mean move the effects into MapData and store that as an entry in the undostack? Instead of adding an Effect as a separate value in a undoEntry?

cpojer · 2024-05-23T17:30:11Z

How do you feel about the previous state (if present) just being restored straight off a reload instead of hitting restore to get back to the previous state?

I prefer not to do this. In the game, opening the map editor should start you off with a fresh map. I mostly just want undo to work properly in that empty state so that we can make the "Restore Map" button more prominent in the editor in a follow-up. That way we give control to the user.

Does this mean move the effects into MapData and store that as an entry in the undostack? Instead of adding an Effect as a separate value in a undoEntry?

I think we have to change undoEntry to take [MapData, Effects] instead of just MapData. Effects cannot live on MapData directly.

cpojer · 2024-08-27T19:59:22Z

Closing due to inactivity. Happy to reopen if it's being picked up again.

tjamc added 2 commits May 17, 2024 15:52

feat: add undo-stack on a per map basis

a2cc8f7

fix: make updateUndoStack param optional

d8c6d8d

cpojer reviewed May 19, 2024

View reviewed changes

tjamc added 3 commits May 19, 2024 19:22

feat: make suggestions

c6f4a04

- change stack keys - remove id from MapCreateVariables type - use null instead of undefined - use array.length - 1 to calc last index

fix: undostack across saves

2c773b5

chore: remove TODO

f5de162

tjamesmac commented May 20, 2024

View reviewed changes

cpojer reviewed May 20, 2024

View reviewed changes

cpojer force-pushed the main branch 3 times, most recently from e7d0373 to e7114fa Compare May 21, 2024 22:39

chore: make amendments

3ce9fed

- nameToSlug -> slug - getUndoStackKey -> getUndoStackKeyFor - getUndoStackKeyIndex -> getUndoStackKeyIndexFor - remove empty line

cpojer closed this Aug 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add undo-stack on a per map basis #22

feat: add undo-stack on a per map basis #22

tjamesmac commented May 17, 2024 •

edited

Loading

cpojer commented May 17, 2024 •

edited

Loading

cpojer May 19, 2024 •

edited

Loading

cpojer May 19, 2024

cpojer May 19, 2024

cpojer May 19, 2024

cpojer commented May 19, 2024

tjamesmac commented May 20, 2024

tjamesmac May 20, 2024

tjamesmac May 20, 2024

cpojer May 20, 2024

cpojer May 20, 2024

cpojer May 20, 2024

cpojer May 20, 2024

tjamesmac May 22, 2024

cpojer May 20, 2024

cpojer commented May 20, 2024

tjamesmac commented May 22, 2024

tjamesmac commented May 22, 2024

cpojer commented May 23, 2024

cpojer commented Aug 27, 2024

	const nameToSlug = toSlug(variables.mapName);
	const slug = toSlug(variables.mapName);

	id: 'id' in variables ? variables.id : nameToSlug,
	id: 'id' in variables ? variables.id : slug,

feat: add undo-stack on a per map basis #22

feat: add undo-stack on a per map basis #22

Conversation

tjamesmac commented May 17, 2024 • edited Loading

Description

Todo

cpojer commented May 17, 2024 • edited Loading

cpojer May 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented May 19, 2024

tjamesmac commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented May 20, 2024

tjamesmac commented May 22, 2024

tjamesmac commented May 22, 2024

cpojer commented May 23, 2024

cpojer commented Aug 27, 2024

tjamesmac commented May 17, 2024 •

edited

Loading

cpojer commented May 17, 2024 •

edited

Loading

cpojer May 19, 2024 •

edited

Loading