Documentation tweaks

Author: aonkeeper4

src/DashStates/DreamTunnelDash/DreamTunnelDash.cs

@@ -429,7 +429,7 @@ private static void NaiveMoveTowardsY(Player player, float targetY, float maxAmo
         player.NaiveMove(Vector2.UnitY * moveY);
     }
 
-    // Utilities to patch any method that checks the player's State
+    // Utilities to patch any method that checks the player's State.
     /// <summary>
     /// Use if decompilation says <c>State==9</c>.
     /// </summary>
@@ -440,17 +440,19 @@ private static void NaiveMoveTowardsY(Player player, float targetY, float maxAmo
     private static readonly ILContext.Manipulator State_DreamDashNotEqual = il => CheckState(new ILCursor(il), Player.StDreamDash, false);
 
     /// <summary>
-    /// Patch any method that checks the player's state.
+    /// Patch any method that checks the player's State.
     /// </summary>
-    /// <remarks>Checks for <c>ldc.i4.s &lt;state&gt;</c></remarks>
+    /// <remarks>Checks for <c>ldc.i4.s &lt;state&gt;</c>.</remarks>
     /// <param name="cursor">The ILCursor to use</param>
     /// <param name="state">The state to check for</param>
-    /// <param name="equal">Whether the decompilation says <c>State == &lt;state&gt;</c></param>
+    /// <param name="equal">Whether to check for <c>== &lt;state&gt;</c> or <c>!= &lt;state&gt;</c></param>
     private static void CheckState(ILCursor cursor, int state, bool equal)
     {
         // essentially, we want to perform these conversions:
-        // `player.StateMachine.State == state` -> `player.StateMachine.State == state || player.StateMachine.State == St.DreamTunnelDash`
-        // `player.StateMachine.State != state` -> `player.StateMachine.State != state && player.StateMachine.State != St.DreamTunnelDash`
+        //  - `player.StateMachine.State == <state>` -> `player.StateMachine.State == St.DreamTunnelDash || player.StateMachine.State == <state>`
+        //  - `player.StateMachine.State != <state>` -> `player.StateMachine.State != St.DreamTunnelDash && player.StateMachine.State != <state>`
+        // the dream tunnel dash check comes before the normal check because 1. it's more predictable to implement and 2. vanilla has no state checks
+        // that cause side effects and would thus be broken by our short-circuiting method.
 
         // go to before the state check
         if (!cursor.TryGotoNextFirstFitReversed(MoveType.AfterLabel, 0x10,
@@ -459,48 +461,50 @@ private static void CheckState(ILCursor cursor, int state, bool equal)
             instr => instr.MatchLdcI4(state)))
             return;
 
-        // variables to grab various things
         bool matchedBeqOrBne = false, matchedCeq = false;
         ILLabel failedCheck = null;
         Instruction ceqInstr = null;
 
-        // retrieve the instruction after the current check
+        // retrieve info about the current check
         ILCursor cloned = cursor.Clone();
         if (!cloned.TryGotoNext(MoveType.After, instr =>
             {
                 // we grab a lot of stuff here: whether we matched a beq/bne.un or a ceq, the "fail state" label of the beq/bne.un
                 // (if we matched one of those) and the actual ceq instruction (if we matched that).
 
-                // equality checks usually use bne.un (for ==) or beq (for !=) in order to branch past the block of the if statement if the values don't match
+                // equality checks usually use bne.un (for ==) or beq (for !=) to branch past the block of the if statement when the values don't match
                 matchedBeqOrBne = equal ? instr.MatchBneUn(out failedCheck) : instr.MatchBeq(out failedCheck);
+                // alternatively they could use ceq and then a brtrue/brfalse to do the same, though i don't think there are any for this purpose in vanilla
                 matchedCeq = (ceqInstr = instr).MatchCeq();
+                
                 return matchedBeqOrBne || matchedCeq;
             })) return;
+        // and the instruction after the current check
         Instruction afterMatch = cloned.Next!;
 
         // beq and bne.un work with labels, whereas ceq just leaves a bool so we need to deal with them differently
         if (matchedBeqOrBne)
         {
-            // labels for cleaning up duplicate player on stack
+            // labels for cleaning up duplicate player left on stack
             ILLabel cleanUpPlayer = cursor.DefineLabel(), pastCleanUpPlayer = cursor.DefineLabel();
 
             // duplicate player on stack
             cursor.Emit(OpCodes.Dup);
-            // check if player is dream tunnel dashing and if so, short-circuit (while also cleaning up the other, now unnecessary duplicate player)
+            // check if player is dream tunnel dashing and if so, short-circuit (while also popping the other, now unnecessary duplicate player off the stack)
             cursor.EmitDelegate<Func<Player, bool>>(player => player.StateMachine.State == St.DreamTunnelDash);
             cursor.Emit(OpCodes.Brtrue, cleanUpPlayer);
             // else, continue with check as normal
 
             // where we short-circuit to depends on whether we check for equality or not.
-            // for equality, we should short-circuit to the "block of the if statement" (past our current condition, whether it be the actual block or another condition),
-            // since our desired behaviour is `player.StateMachine.State == state || player.StateMachine.State == St.DreamTunnelDash` and the first part of that or has been satisfied,
+            // for equality, we should short-circuit to the "block" of the if statement (past our current condition, whether it be the actual block or another condition),
+            // since our desired behaviour is `player.StateMachine.State == state || player.StateMachine.State == St.DreamTunnelDash` and the first term of that or has been satisfied,
             // just like how `if (true || condition()) { ... }` should immediately skip checking `condition()` (since `true` or anything is `true`) and run the block inside the if.
-            // for inequality, it's the other way around: we should short-circuit immediately past the rest of the if statement, since our desired behaviour will be
+            // for inequality, it's the other way around: we should short-circuit past the rest of the if statement (skipping the block), since our desired behaviour will be
             // `player.StateMachine.State != state && player.StateMachine.State != St.DreamTunnelDash` and we know the first condition of that and is false, just like how
             // `if (false && condition()) { ... }` should immediately skip checking `condition()` (since `false` and anything is `false`) and never run the block inside the if.
             // our `afterMatch` label points to after our current condition, and our `failedCheck` label points to after the rest of the statement.
             cursor.Goto(equal ? afterMatch : failedCheck.Target);
-            // extra player cleanup, we branch over it in normal behaviour but jump into it if needed (see above)
+            // extra player cleanup, we skip over it in normal behaviour but jump into it if needed (see above)
             cursor.Emit(OpCodes.Br, pastCleanUpPlayer);
             cursor.Emit(OpCodes.Pop);
             cursor.MarkLabel(pastCleanUpPlayer);