Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Achievement Bit fixes #289

Merged
merged 3 commits into from
May 20, 2024
Merged

Achievement Bit fixes #289

merged 3 commits into from
May 20, 2024

Conversation

AsherGlick
Copy link
Owner

@AsherGlick AsherGlick commented May 19, 2024

We had previously assumed that achievement bit was a bitmask, but it is an index.
This PR fixes this misconception and puts effort to prevent it from occuring again by

  • Renaming the proto filed to bit_index
  • Adding an xml field option of AchievementBitIndex as an alternative to AchievementBit
  • Removing or renaming anything that used achievement bitmask

This PR will have wonky diffs, the best way to review this is to review it one commit at a time. The first commit is the real diff, the second commit is just renaming files.

Additionally the protobin diffs are going to be very strange. This is for two reasons

  • The proto diff code does not use the most up-to-date version of the protodef because it runs on the branch we are trying to merge into instead of the branch we are trying to merge
  • There seems to also be a bug with some of the negative fixed32 numbers where textproto is interpreting them as unsigned 64 bit numbers
  • I committed a cardinal sin of re-indexing the proto fields. 16 used to be achievement bit, but now it is achievement. 17 used to be achievement but is now achievement_bit
  • That cardinal sin is fine because we still have not released this proto. Anyone who is using it already should be ready to suffer the consequences of it changing like this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- xml_converter/integration_tests/test_cases/achievement_bit_index_valid/output_proto/markers.bin.textproto._old	2024-05-19 02:44:24.571726348 +0000
+++ xml_converter/integration_tests/test_cases/achievement_bit_index_valid/output_proto/markers.bin.textproto._new	2024-05-19 02:44:24.579726476 +0000
@@ -0,0 +1,42 @@
+category {
+  name: "My Category"
+  icon {
+    map_id: 50
+    position {
+      x: 169.81
+      y: 210.65
+      z: 215.83
+    }
+  }
+  icon {
+    map_id: 50
+    position {
+      x: 169.81
+      y: 210.65
+      z: 215.83
+    }
+    achievement_id: 5
+  }
+  icon {
+    map_id: 50
+    position {
+      x: 169.81
+      y: 210.65
+      z: 215.83
+    }
+    achievement_id: 2147483647
+  }
+  icon {
+    map_id: 50
+    position {
+      x: 169.81
+      y: 210.65
+      z: 215.83
+    }
+    achievement_id: 2147483647
+  }
+  trail {
+    achievement_id: 5
+  }
+  id: "(\350\314\006\302\223^\226"
+}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- xml_converter/integration_tests/test_cases/achievement_bitmask_valid/output_proto/markers.bin.textproto._old	2024-05-19 02:44:24.595726731 +0000
+++ xml_converter/integration_tests/test_cases/achievement_bitmask_valid/output_proto/markers.bin.textproto._new	2024-05-19 02:44:24.595726731 +0000
@@ -1,51 +0,0 @@
-category {
-  name: "My Category"
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-  }
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-    achievement_bit: 5
-  }
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-    achievement_bit: 2147483647
-  }
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-    achievement_bit: 2147483648
-  }
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-    achievement_bit: 4294967291
-  }
-  trail {
-    achievement_bit: 5
-  }
-  id: "(\350\314\006\302\223^\226"
-}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- xml_converter/integration_tests/test_cases/achievement_id/output_proto/markers.bin.textproto._old	2024-05-19 02:44:24.607726922 +0000
+++ xml_converter/integration_tests/test_cases/achievement_id/output_proto/markers.bin.textproto._new	2024-05-19 02:44:24.615727050 +0000
@@ -15,7 +15,7 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: 1
+    16: 1
   }
   icon {
     map_id: 50
@@ -24,7 +24,7 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: 5
+    16: 5
   }
   icon {
     map_id: 50
@@ -33,7 +33,7 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: 2147483647
+    16: 2147483647
   }
   icon {
     map_id: 50
@@ -42,7 +42,7 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: -2147483648
+    16: 18446744071562067968
   }
   icon {
     map_id: 50
@@ -51,10 +51,10 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: -5
+    16: 18446744073709551611
   }
   trail {
-    achievement_id: 5
+    16: 5
   }
   id: "(\350\314\006\302\223^\226"
 }

Base automatically changed from documentation_examples to xml_converter May 20, 2024 14:57

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- xml_converter/integration_tests/test_cases/achievement_bit_index_valid/output_proto/markers.bin.textproto._old	2024-05-20 15:05:19.527211817 +0000
+++ xml_converter/integration_tests/test_cases/achievement_bit_index_valid/output_proto/markers.bin.textproto._new	2024-05-20 15:05:19.535211866 +0000
@@ -0,0 +1,33 @@
+category {
+  name: "My Category"
+  icon {
+    map_id: 50
+    position {
+      x: 169.81
+      y: 210.65
+      z: 215.83
+    }
+  }
+  icon {
+    map_id: 50
+    position {
+      x: 169.81
+      y: 210.65
+      z: 215.83
+    }
+    achievement_id: 5
+  }
+  icon {
+    map_id: 50
+    position {
+      x: 169.81
+      y: 210.65
+      z: 215.83
+    }
+    achievement_id: 2147483647
+  }
+  trail {
+    achievement_id: 5
+  }
+  id: "(\350\314\006\302\223^\226"
+}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- xml_converter/integration_tests/test_cases/achievement_bitmask_valid/output_proto/markers.bin.textproto._old	2024-05-20 15:05:19.547211938 +0000
+++ xml_converter/integration_tests/test_cases/achievement_bitmask_valid/output_proto/markers.bin.textproto._new	2024-05-20 15:05:19.551211962 +0000
@@ -1,51 +0,0 @@
-category {
-  name: "My Category"
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-  }
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-    achievement_bit: 5
-  }
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-    achievement_bit: 2147483647
-  }
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-    achievement_bit: 2147483648
-  }
-  icon {
-    map_id: 50
-    position {
-      x: 169.81
-      y: 210.65
-      z: 215.83
-    }
-    achievement_bit: 4294967291
-  }
-  trail {
-    achievement_bit: 5
-  }
-  id: "(\350\314\006\302\223^\226"
-}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- xml_converter/integration_tests/test_cases/achievement_id/output_proto/markers.bin.textproto._old	2024-05-20 15:05:19.563212035 +0000
+++ xml_converter/integration_tests/test_cases/achievement_id/output_proto/markers.bin.textproto._new	2024-05-20 15:05:19.571212083 +0000
@@ -15,7 +15,7 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: 1
+    16: 1
   }
   icon {
     map_id: 50
@@ -24,7 +24,7 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: 5
+    16: 5
   }
   icon {
     map_id: 50
@@ -33,7 +33,7 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: 2147483647
+    16: 2147483647
   }
   icon {
     map_id: 50
@@ -42,7 +42,7 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: -2147483648
+    16: 18446744071562067968
   }
   icon {
     map_id: 50
@@ -51,10 +51,10 @@
       y: 210.65
       z: 215.83
     }
-    achievement_id: -5
+    16: 18446744073709551611
   }
   trail {
-    achievement_id: 5
+    16: 5
   }
   id: "(\350\314\006\302\223^\226"
 }

@AsherGlick AsherGlick merged commit c55b29f into xml_converter May 20, 2024
13 checks passed
@AsherGlick AsherGlick deleted the achivement_bit_fixes branch May 20, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants