Symmetrical Drawing Mode in the Map Editor by dkratz · Pull Request #19 · nkzw-tech/athena-crisis

dkratz · 2024-05-15T14:42:02Z

Description

Adds symmetrical drawing mode in the map editor.

Resolves #7

cpojer · 2024-05-16T16:41:28Z

  }

+  mirror(
+    mapSize: SizeVector,


There is some repetition in this function, so maybe just destructure like { height, width }: SizeVector?

Actually, since this function is only used sparely, I would probably prefer making it a standalone function in athena/lib or hera/editor/lib as like mirrorVector(vector: Vector, mirrorType: MirrorType).

349ef4c
091a44e

cpojer · 2024-05-16T16:45:31Z

@@ -1,4 +1,6 @@
 import sortBy from '@deities/hephaestus/sortBy.tsx';
+import { DrawingMode } from '../../hera/editor/Types.tsx';


We can't import from hera in the athena package since athena also runs on the server without access to React. I think we need to define a VectorMirror (?) type with just 'horizontal' | 'vertical' | 'diagonal' and then use that as a base for DrawingMode in the editor types.

This should be solved, since I moved vector.mirror inside hera: 349ef4c

cpojer · 2024-05-16T16:46:05Z

 import MenuBehavior from './behavior/Menu.tsx';
 import NullBehavior from './behavior/NullBehavior.tsx';
 import Cursor from './Cursor.tsx';
+import MapEditorExtraCursors from './editor/MapEditorExtraCursors.tsx';


What about calling it MapEditorMirrorCursors.tsx?

👍 430a4f7

cpojer · 2024-05-16T16:46:43Z

+  private putMultiple(
+    vectors: Vector[],
+    state: State,
+    actions: Actions,
+    editor: EditorState,
+  ): StateLike | null {
+    let result: StateLike | null = null;
+    const players = Array.from(
+      new Set([...state.map.active, ...PlayerIDs]),
+    ).slice(0, vectors.length);
+    vectors.forEach((vector, index) => {
+      const currentPlayerIndex = players.indexOf(
+        state.map.getCurrentPlayer().id,
+      );
+      const playerId =
+        players[
+          ((currentPlayerIndex >= 0 ? currentPlayerIndex : 0) + index) %
+            players.length
+        ];
+
+      const tempResult = this.put(
+        vector,
+        { ...state, ...result },
+        actions,
+        editor,
+        playerId,
+      );
+      result = {
+        ...(result ?? {}),
+        ...tempResult,
+      };
+    });
+    return result;
+  }


This is why more elegant than what I could have come up with. Nice!

cpojer · 2024-05-16T16:48:41Z

+  }
+
+  return vectors.map((vector) => (
+    <Cursor key={vector.toString()} {...props} position={vector} />


You can just pass vector, toString is implicitly called.

TypeScript doesn't like that:
Type 'Vector' is not assignable to type 'Key | null | undefined'.ts(2322)

cpojer · 2024-05-16T16:50:46Z

+  drawingMode: DrawingMode,
+  mapSize: SizeVector,
+) {
+  const vectors: Vector[] = [];


Nit: Code style.

Suggested change

const vectors: Vector[] = [];

const vectors: Array<Vector> = [];

Updated: b8ece22

Also opened #21

cpojer · 2024-05-16T16:52:00Z

+    }
+  }
+
+  return vectors.filter((v) => !vector.equals(v));


Nit: I'm always trying to have descriptive variable names, even if it's obvious. In this case, I would probably rename vector to origin, and then rename v here to vector. That makes the code just slightly easier to read overall.

👍
d4f9a4c 0637a71

cpojer · 2024-05-16T16:53:10Z

+      const playerId =
+        players[
+          ((currentPlayerIndex >= 0 ? currentPlayerIndex : 0) + index) %
+            players.length
+        ];


This should not include 0 as 0 is the neutral unit type that can be rescued. Can you update it so it only uses one of the valid player ids except 0?

cpojer · 2024-05-16T16:54:36Z

+      const tempResult = this.put(
+        vector,
+        { ...state, ...result },
+        actions,
+        editor,
+        playerId,
+      );
+      result = {
+        ...(result ?? {}),
+        ...tempResult,
+      };


You shouldn't need the ?? {} as you can spread null.

Suggested change

const tempResult = this.put(

vector,

{ ...state, ...result },

actions,

editor,

playerId,

);

result = {

...(result ?? {}),

...tempResult,

};

const newState = this.put(

vector,

{ ...state, ...result },

actions,

editor,

playerId,

);

result = {

...(newState),

...tempResult,

};

I think result had a different type at some point. Removed the null check: e9eaa9a

Some renaming: 387fe45

cpojer · 2024-05-16T16:58:16Z

Thank you for the contribution, the implementation is so elegant and it feels magical already. Apologies that it took me a bit to get back to you as I'm at a conference right now. I left a few comments, but it's just small nits.

Replying to your comment on the issue here:

Do you have a preference on where to put the buttons? I was thinking of placing them below the "Playtest" button on the right side (vertically stacked).

I think that would look best too. I still need to think about the exact icons, but if you can get the layout ready I'll take another look and push some icons.

cpojer · 2024-05-16T16:59:32Z

    <Tick animationConfig={AnimationConfig}>
      <Stack gap={24} vertical verticalPadding>
+        <Box center gap={32}>
+          {/* TODO(dkratz): Remove text and add selected state to buttons */}


You can either use selected or selectedText on InlineLink. They are different active states for links. You can use the editor's drawing mode to compare the state.

Done: bdf6ab5

cpojer · 2024-05-16T17:02:15Z

On maps with odd width/height, cursor artifacts can be left behind like this:

Repro is just to draw randomly on the map for a bit and then the artifact cursors show up.

dkratz · 2024-05-17T12:22:23Z

On maps with odd width/height, cursor artifacts can be left behind like this:

Repro is just to draw randomly on the map for a bit and then the artifact cursors show up.

This seems to happen in the horizontal-vertical mode by simply moving the mouse around (no need to draw).
Not memoizing MapEditorMirrorCurors fixes that issue (although I'm not quite sure why, not memoizing it should be fine though.).

dkratz · 2024-05-17T13:18:50Z

+                      // TODO: adjust color
+                      color="red"


@cpojer PTAL

I think I'm just gong to leave them all red for now. It's easier to parse. Might change it in the future.

cpojer · 2024-05-19T04:24:12Z

The problem with the "stuck" cursors was that getSymmetricalPositions sometimes return an array containing the same vector multiple times. I'm fixing that up.

cpojer · 2024-05-19T04:49:10Z

Thank you, this is awesome, I'm very excited about this feature! The funds were sent to you. I'd love to see more contributions if you are working on any!

dkratz · 2024-05-19T05:53:59Z

Nice, excited to have my first open source contribution merged :)

dkratz added 6 commits May 15, 2024 23:01

Add symmetric drawing mode

9e42448

Add extra cursors

3a74945

memo MapEditorExtraCursors

24e0e72

Move TODO

10f5676

Rename SymmetricDrawingMode to DrawingMode

f1ae644

Only show multiple cursors in design mode

18f8701

dkratz mentioned this pull request May 15, 2024

[Feature] Symmetrical Drawing Mode in the Map Editor #7

Closed

dkratz added 3 commits May 16, 2024 19:06

Rotate player ids for units and buildings

fefbfa7

getSymmetricPositions tsx to ts

bf54192

Remove duplicated vectors

362601e

cpojer reviewed May 16, 2024

View reviewed changes

dkratz added 8 commits May 17, 2024 17:35

Add getSymmetricPositions.test test

5263300

Move Vector.mirror to mirrorVector

349ef4c

Cleanup mirrorVector

091a44e

Rename to MapEditorMirrorCursors

430a4f7

code style

b8ece22

Rename args

d4f9a4c

fix error

0637a71

Don't use 0 as player ID when placing multiple

ec683d3

dkratz added 2 commits May 17, 2024 18:55

Remove unnecessary null check

e9eaa9a

Rename

387fe45

dkratz mentioned this pull request May 17, 2024

Add @typescript-eslint/array-type eslint rule #21

Merged

Don't memoize MapEditorMirrorCursors

306bcc3

dkratz added 5 commits May 17, 2024 21:42

Layout

6c8bbb9

Add button selected states

bdf6ab5

Update TODO text

23fabe5

Fix formatting

856d25d

pnpm format

5da7b6a

dkratz commented May 17, 2024

View reviewed changes

dkratz requested a review from cpojer May 17, 2024 13:25

dkratz marked this pull request as ready for review May 17, 2024 13:25

Polish and small cleanups.

9210f69

cpojer force-pushed the map-editor-symmetric-drawing branch from 0056017 to 9210f69 Compare May 19, 2024 04:44

cpojer merged commit e893189 into nkzw-tech:main May 19, 2024

		@@ -1,4 +1,6 @@
		import sortBy from '@deities/hephaestus/sortBy.tsx';
		import { DrawingMode } from '../../hera/editor/Types.tsx';

	const vectors: Vector[] = [];
	const vectors: Array<Vector> = [];

Uh oh!

Conversation

dkratz commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpojer commented May 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpojer commented May 16, 2024

Uh oh!

dkratz commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cpojer commented May 19, 2024

Uh oh!

cpojer commented May 19, 2024

Uh oh!

dkratz commented May 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dkratz commented May 15, 2024 •

edited

Loading

dkratz commented May 17, 2024 •

edited

Loading