Map conversion checks and mapmerge2 fixup rework (#7595)

# About the pull request

This PR is an experimental change to mapmerge2 where now free keys are
gotten in sequential order rather than random, the map linter now
attempts to merge all maps to determine if mapmerge2 would want to alter
the maps (to detect a failure of using hooks or non-tgm format), and
hopefully a more robust fixup script.

The other changes to maplint are to allow it to more gracefully handle
var edits to areas (we already lint against this but it won't be
properly handled), and var edits as
[newlist](https://www.byond.com/docs/ref/#/proc/newlist).

Fixup now forces the upstream remote to be added to their repo (I don't
know of a different way to get the merge_base I want), merges based off
of the merge_base version of the map (rather than really only looking at
the HEAD commit), detects mapmerge2 changes for any map the HEAD tree is
altering, and performs them. However, fixup does take significantly more
time, but it really only checked for TGM format previously...

I also tweaked all [ci skip] checks to suppress the errors linted in
VSC.

# Explain why it's good for the game
Hopefully less mapping conflicts in the scenario when a contributor is
not using the
[hooks](https://github.com/cmss13-devs/cmss13/tree/master/tools/hooks).

# Testing Photographs and Procedure
See the commit history of me testing changes as well as images below...

<details>
<summary>Screenshots & Videos</summary>

Commiting a map w/o hooks (and various commits in between):

![image](https://github.com/user-attachments/assets/23f7b5d1-dd43-4796-965c-826f8e41bccc)

Fixup script:

![image](https://github.com/user-attachments/assets/61db20ef-bb9c-43d5-a39b-80908dfb7986)

And then again:

![image](https://github.com/user-attachments/assets/b58c26c1-9b1b-43e2-be19-a9f114bbd4b0)

A new map in DMM format:

![image](https://github.com/user-attachments/assets/a7ab03d7-f7fd-468b-9845-d6e3c65e873c)

![image](https://github.com/user-attachments/assets/f96cfdc2-238c-47b2-8230-ef916593af89)

</details>

# Changelog
:cl:
code: dmm_test now checks if there are pending mapmerge2 conversions.
code: Improved dmm_test error handling
code: mapmerge2 now uses keys sequentially rather than randomly.
code: mapmerge2 fixup script now assigns upstream remote if needed, and
checks/fixes pending mapmerge2 conversions.
/:cl:

.github/workflows/ci_suite.yml

@@ -6,29 +6,31 @@ on:
   merge_group:
 jobs:
   run_linters:
-    if: "!contains(github.event.head_commit.message, '[ci skip]')"
+    if: ( !contains(github.event.head_commit.message, '[ci skip]') )
     name: Run Linters
     runs-on: ubuntu-latest
     concurrency:
       group: run_linters-${{ github.head_ref || github.run_id }}
       cancel-in-progress: true
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
       - name: Restore SpacemanDMM cache
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: ~/SpacemanDMM
           key: ${{ runner.os }}-spacemandmm-${{ secrets.CACHE_PURGE_KEY }}
       - name: Restore Yarn cache
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: tgui/.yarn/cache
           key: ${{ runner.os }}-yarn-${{ secrets.CACHE_PURGE_KEY }}-${{ hashFiles('tgui/yarn.lock') }}
           restore-keys: |
             ${{ runner.os }}-build-
             ${{ runner.os }}-
       - name: Restore Rust cache
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: ~/.cargo
           key: ${{ runner.os }}-rust
@@ -75,7 +77,7 @@ jobs:
       - run: ./DMCompiler_linux-x64/DMCompiler --suppress-unimplemented colonialmarines.dme
 
   compile_all_maps:
-    if: "!contains(github.event.head_commit.message, '[ci skip]')"
+    if: ( !contains(github.event.head_commit.message, '[ci skip]') )
     name: Compile Maps
     runs-on: ubuntu-latest
     steps:
@@ -92,7 +94,7 @@ jobs:
           tools/build/build --ci dm -DCIBUILDING -DCITESTING -DALL_MAPS
 
   find_all_maps:
-    if: "!contains(github.event.head_commit.message, '[ci skip]')"
+    if: ( !contains(github.event.head_commit.message, '[ci skip]') )
     name: Find Maps to Test
     runs-on: ubuntu-latest
     outputs:
@@ -117,7 +119,7 @@ jobs:
           ALTERNATE_TESTS_JSON=$(jq -nRc '[inputs | capture("^(?<major>[0-9]+)\\.(?<minor>[0-9]+): (?<map>.+)$")]' .github/alternate_byond_versions.txt)
           echo "alternate_tests=$ALTERNATE_TESTS_JSON" >> $GITHUB_OUTPUT
   run_all_tests:
-    if: "!contains(github.event.head_commit.message, '[ci skip]')"
+    if: ( !contains(github.event.head_commit.message, '[ci skip]') )
     name: Unit Tests
     needs: [find_all_maps]
     strategy:
@@ -132,7 +134,7 @@ jobs:
       map: ${{ matrix.map }}
 
   run_alternate_tests:
-    if: "!contains(github.event.head_commit.message, '[ci skip]') && needs.find_all_maps.outputs.alternate_tests != '[]'"
+    if: ( !contains(github.event.head_commit.message, '[ci skip]') && needs.find_all_maps.outputs.alternate_tests != '[]' )
     name: Alternate Tests
     needs: [find_all_maps]
     strategy:
@@ -149,15 +151,15 @@ jobs:
       minor: ${{ matrix.setup.minor }}
 
   check_alternate_tests:
-    if: "!contains(github.event.head_commit.message, '[ci skip]') && needs.find_all_maps.outputs.alternate_tests != '[]'"
+    if: ( !contains(github.event.head_commit.message, '[ci skip]') && needs.find_all_maps.outputs.alternate_tests != '[]' )
     name: Check Alternate Tests
     needs: [run_alternate_tests]
     runs-on: ubuntu-latest
     steps:
       - run: echo Alternate tests passed.
 
   test_windows:
-    if: "!contains(github.event.head_commit.message, '[ci skip]')"
+    if: ( !contains(github.event.head_commit.message, '[ci skip]') )
     name: Windows Build
     runs-on: windows-latest
     concurrency:

.github/workflows/generate_documentation.yml

@@ -5,7 +5,7 @@ on:
     - master
 jobs:
   generate_documentation:
-    if: "!contains(github.event.head_commit.message, '[ci skip]')"
+    if: ( !contains(github.event.head_commit.message, '[ci skip]') )
     runs-on: ubuntu-latest
     concurrency: gen-docs
     steps:

tools/maplint/source/dmm.py

@@ -94,7 +94,12 @@ def parse_pop(self):
                 break
             elif content_end == "{":
                 while (var_edit := self.parse_var_edit()) is not None:
+                    if var_edit[0] == None and var_edit[1] == None:
+                        break
                     content.var_edits[var_edit[0]] = var_edit[1]
+                else:
+                    continue # inner loop didn't break
+                break # inner loop did break indicating a })
             elif content_end == ",":
                 continue
 
@@ -106,6 +111,8 @@ def parse_var_edit(self):
         line = self.next_line()
         if line == "\t},":
             return None
+        if line == "\t})":
+            return None, None
 
         var_edit_match = REGEX_VAR_EDIT.match(line)
         self.expect(var_edit_match is not None, "Var edits ended too early, expected a newline in between.")
@@ -128,6 +135,8 @@ def parse_constant(self, constant):
             return Filename(constant[1:-1])
         elif (list_match := re.match(r'^list\((?P<contents>.*)\)$', constant)):
             return ["NYI: list"]
+        elif (list_match := re.match(r'^newlist\((?P<contents>.*)\)$', constant)):
+            return ["NYI: newlist"]
         else:
             self.raise_error(f"Unknown constant type: {constant}")
 

tools/mapmerge2/dmm.py

@@ -11,14 +11,15 @@
 Coordinate = namedtuple('Coordinate', ['x', 'y', 'z'])
 
 class DMM:
-    __slots__ = ['key_length', 'size', 'dictionary', 'grid', 'header']
+    __slots__ = ['key_length', 'size', 'dictionary', 'grid', 'header', 'lowest_free']
 
     def __init__(self, key_length, size):
         self.key_length = key_length
         self.size = size
         self.dictionary = bidict.bidict()
         self.grid = {}
         self.header = None
+        self.lowest_free = 0
 
     @staticmethod
     def from_file(fname):
@@ -60,10 +61,14 @@ def set_tile(self, coord, tile):
     def generate_new_key(self):
         self._ensure_free_keys(1)
         max_key = max_key_for(self.key_length)
-        # choose one of the free keys at random
-        key = random.randint(0, max_key - 1)
+        # choose one of the free keys
+        key = self.lowest_free
         while key in self.dictionary:
-            key = random.randint(0, max_key - 1)
+            key = key + 1
+            if key >= max_key:
+                print("The value for lowest_free was invalid!")
+                key = 0
+        self.lowest_free = key
         return key
 
     def overwrite_key(self, key, fixed, bad_keys):
@@ -90,6 +95,7 @@ def remove_unused_keys(self, modified_keys = None):
                 unused_keys.remove(key)
         for key in unused_keys:
             del self.dictionary[key]
+        self.lowest_free = 0
 
     def _presave_checks(self):
         # last-second handling of bogus keys to help prevent and fix broken maps

tools/mapmerge2/dmm_test.py

@@ -1,25 +1,88 @@
 import os
 import sys
+import pygit2
+
 from .dmm import *
+from .mapmerge import merge_map
+
+def green(text):
+    return "\033[32m" + str(text) + "\033[0m"
+
+def red(text):
+    return "\033[31m" + str(text) + "\033[0m"
+
+def has_tgm_header(fname):
+    with open(fname, 'r', encoding=ENCODING) as f:
+        data = f.read(len(TGM_HEADER))
+        return data.startswith(TGM_HEADER)
 
+class LintException(Exception):
+    pass
 
 def _self_test():
-    # test: can we load every DMM in the tree
+    repo = pygit2.Repository(pygit2.discover_repository(os.getcwd()))
+
+    # Read the HEAD and ancestor commits
+    # Assumption: origin on the runner is what we'd normally call upstream
+    ancestor = repo.merge_base(repo.head.target, repo.revparse_single("refs/remotes/origin/master").id)
+    ancestor_commit = None
+    if not ancestor:
+        print("Unable to determine merge base!")
+    else:
+        ancestor_commit = repo[ancestor]
+        print("Determined ancestor commit SHA to be:", ancestor)
+
     count = 0
+    failed = 0
     for dirpath, dirnames, filenames in os.walk('.'):
         if '.git' in dirnames:
             dirnames.remove('.git')
         for filename in filenames:
             if filename.endswith('.dmm'):
                 fullpath = os.path.join(dirpath, filename)
+                path = fullpath.replace("\\", "/").removeprefix("./")
                 try:
-                    DMM.from_file(fullpath)
+                    # test: can we load every DMM
+                    index_map = DMM.from_file(fullpath)
+
+                    # test: is every DMM in TGM format
+                    if not has_tgm_header(fullpath):
+                        raise LintException('Map is not in TGM format! Please run `/tools/mapmerge2/I Forgot To Map Merge.bat`')
+
+                    # test: does every DMM convert cleanly
+                    if ancestor_commit:
+                        try:
+                            ancestor_blob = ancestor_commit.tree[path]
+                        except KeyError:
+                            # New map, no entry in ancestor
+                            print("New map? Could not find ancestor version of", path)
+                            merged_map = merge_map(index_map, index_map) # basically only tests unused keys
+                            original_bytes = index_map.to_bytes()
+                            merged_bytes = merged_map.to_bytes()
+                            if original_bytes != merged_bytes:
+                                raise LintException('New map is pending updates! Please run `/tools/mapmerge2/I Forgot To Map Merge.bat`')
+                        else:
+                            # Entry in ancestor, merge the index over it
+                            ancestor_map = DMM.from_bytes(ancestor_blob.read_raw())
+                            merged_map = merge_map(index_map, ancestor_map)
+                            original_bytes = index_map.to_bytes()
+                            merged_bytes = merged_map.to_bytes()
+                            if original_bytes != merged_bytes:
+                                raise LintException('Map is pending updates! Please run `/tools/mapmerge2/I Forgot To Map Merge.bat`')
+                except LintException as error:
+                    failed += 1
+                    print(red(f'Failed on: {path}'))
+                    print(error)
                 except Exception:
-                    print('Failed on:', fullpath)
+                    failed += 1
+                    print(red(f'Failed on: {path}'))
                     raise
                 count += 1
 
-    print(f"{os.path.relpath(__file__)}: successfully parsed {count} .dmm files")
+    print(f"{os.path.relpath(__file__)}: {green(f'successfully parsed {count-failed} .dmm files')}")
+    if failed > 0:
+        print(f"{os.path.relpath(__file__)}: {red(f'failed to parse {failed} .dmm files')}")
+        exit(1)
 
 
 def _usage():

tools/mapmerge2/fixup.py

@@ -40,7 +40,7 @@ def insert_into_tree(repo, tree_builder, path, blob_oid):
         tree_builder.insert(first, inner.write(), pygit2.GIT_FILEMODE_TREE)
 
 
-def main(repo):
+def main(repo : pygit2.Repository):
     if repo.index.conflicts:
         print("You need to resolve merge conflicts first.")
         return 1
@@ -51,54 +51,65 @@ def main(repo):
             continue
         if status & STATUS_INDEX:
             print("You have changes staged for commit. Commit them or unstage them first.")
-            print("If you are about to commit maps for the first time, run `Run Before Committing.bat`.")
+            print("If you are about to commit maps for the first time and can't use hooks, run `Run Before Committing.bat`.")
             return 1
         if path.endswith(".dmm") and (status & STATUS_WT):
             print("You have modified maps. Commit them first.")
-            print("If you are about to commit maps for the first time, run `Run Before Committing.bat`.")
+            print("If you are about to commit maps for the first time and can't use hooks, run `Run Before Committing.bat`.")
             return 1
 
-    # Read the HEAD commit.
-    head_commit = repo[repo.head.target]
-    head_files = {}
-    for path, blob in walk_tree(head_commit.tree):
-        if path.endswith(".dmm"):
-            data = blob.read_raw()
-            if not data.startswith(TGM_HEADER):
-                head_files[path] = dmm.DMM.from_bytes(data)
+    # Set up upstream remote if needed
+    try:
+        repo.remotes.create("upstream", "https://github.com/cmss13-devs/cmss13.git")
+    except ValueError:
+        pass
+    else:
+        print("Adding upstream remote...")
 
-    if not head_files:
-        print("All committed maps appear to be in the correct format.")
-        print("If you are about to commit maps for the first time, run `Run Before Committing.bat`.")
+    # Read the HEAD and ancestor commits.
+    head_commit = repo[repo.head.target]
+    repo.remotes["upstream"].fetch()
+    ancestor = repo.merge_base(repo.head.target, repo.revparse_single("refs/remotes/upstream/master").id)
+    if not ancestor:
+        print("Unable to automatically fix anything because merge base could not be determined.")
         return 1
+    ancestor_commit = repo[ancestor]
+    print("Determined ancestor commit SHA to be:", ancestor)
 
-    # Work backwards to find a base for each map, converting as found.
+    # Walk the head commit tree and convert as needed
     converted = {}
-    if len(head_commit.parents) != 1:
-        print("Unable to automatically fix anything because HEAD is a merge commit.")
-        return 1
     commit_message_lines = []
-    working_commit = head_commit.parents[0]
-    while len(converted) < len(head_files):
-        for path in head_files.keys() - converted.keys():
+    for path, blob in walk_tree(head_commit.tree):
+        if path.endswith(".dmm"):
+            head_data = blob.read_raw()
+            head_map = dmm.DMM.from_bytes(head_data)
+
+            # test: does every DMM convert cleanly (including TGM conversion)
             try:
-                blob = working_commit.tree[path]
+                ancestor_blob = ancestor_commit.tree[path]
             except KeyError:
-                commit_message_lines.append(f"{'new':{ABBREV_LEN}}: {path}")
-                print(f"Converting new map: {path}")
-                converted[path] = head_files[path]
+                # New map, no entry in ancestor
+                merged_map = merge_map(head_map, head_map)
+                merged_bytes = merged_map.to_bytes()
+                if head_data != merged_bytes:
+                    print(f"Updating new map: {path}")
+                    commit_message_lines.append(f"{'new':{ABBREV_LEN}}: {path}")
+                    converted[path] = merged_map
             else:
-                data = blob.read_raw()
-                if data.startswith(TGM_HEADER):
-                    str_id = str(working_commit.id)[:ABBREV_LEN]
+                # Entry in ancestor
+                ancestor_map = dmm.DMM.from_bytes(ancestor_blob.read_raw())
+                merged_map = merge_map(head_map, ancestor_map)
+                merged_bytes = merged_map.to_bytes()
+                if head_data != merged_bytes:
+                    print(f"Updating map: {path}")
+                    str_id = str(ancestor_commit.id)[:ABBREV_LEN]
                     commit_message_lines.append(f"{str_id}: {path}")
-                    print(f"Converting map: {path}")
-                    converted[path] = merge_map(head_files[path], dmm.DMM.from_bytes(data))
-        if len(working_commit.parents) != 1:
-            print("A merge commit was encountered before good versions of these maps were found:")
-            print("\n".join(f"    {x}" for x in head_files.keys() - converted.keys()))
-            return 1
-        working_commit = working_commit.parents[0]
+                    converted[path] = merged_map
+
+    if not converted:
+        print("All committed maps appear to be okay.")
+        print("If you are about to commit maps for the first time and can't use hooks, run `Run Before Committing.bat`.")
+        return 1
 
     # Okay, do the actual work.
     tree_builder = repo.TreeBuilder(head_commit.tree)
@@ -118,7 +129,7 @@ def main(repo):
         repo.head.name,
         signature,  # author
         signature,  # committer
-        f'Convert maps to TGM\n\n{joined}\n\nAutomatically commited by: {os.path.relpath(__file__, repo.workdir)}',
+        f'Fixup maps in TGM format\n\n{joined}\n\nAutomatically commited by: {os.path.relpath(__file__, repo.workdir)}',
         tree_builder.write(),
         [head_commit.id],
     )