Fix Inverted Song of Time better

This fixes severe issues with NPCs with the older ISoT fix.

The previous patch used to set the time speed to -2 to stop time
every 6 frames when it should have been done every 3 frames to get
a 1/3 ratio.

However, even with that fixed, scheduled NPCs were affected by a
serious problem: their animations would be very janky looking and
distracting. The reason the previous change broke NPCs is that they
internally use the time speed to determine their walking
animation speed.

The natural fix -- keeping a constant time speed but manually
decreasing the time variable every 3 frames -- does solve that issue,
but causes NPCs to reach their destination too fast and spend a lot
of time waiting in front of doors.

In order to avoid that problem, I hooked the function that is
responsible for moving scheduled NPCs based on the time speed and
manually applied the 1/3 slow time multiplier. Surprisingly, this was
enough to completely fix the waiting issue... and of course this also
introduced yet another glitch.

NPCs were now moving backwards and facing the wrong direction whenever
a loading zone was activated... After one day of debugging I eventually
figured out that this is because the schedule position variable is
truncated every time it is supposed to stop updating (whenever time
is frozen for instance). This would normally not be an issue -- as the
variable only used to store integer values -- but our ISoT fix requires
the use of non-integer speeds, so the truncation becomes an obstacle
to restoring the ISoT.

The fix -- or rather the hacky workaround -- is to try to detect
position truncation and restore the last known good value.

Pointers to positions are registered by the "move scheduled NPC" hook;
every tick our own code hopes that the pointers are still valid,
looks at the current values and overwrites them if needed.
Not proud of this solution, but I don't see a better way to do it
other than manually patching every single NPC actor (which I don't
want to do).

Fixes issue #4

readme.md

@@ -22,7 +22,7 @@ so playing on a New 3DS/2DS or Citra is recommended for a better experience.
 * **Fast Ocarina**: Dedicated physical button for the Ocarina of Time
   * Press ZR to play the instrument.
 
-* ~~**More Effective Inverted Song of Time**: The ISoT makes time go at 1/3 speed (rather than 1/2).~~ Temporarily disabled because of issues with some NPCs.
+* **More Effective Inverted Song of Time**: Slow time to 1/3 speed (as in the original)
   * Makes some glitchless challenge runs possible again.
   * Gives the player more time in a three-day cycle.
 

source/CMakeLists.txt

@@ -26,6 +26,7 @@ add_executable(newcode
   game/player.h
   game/static_context.cpp
   game/static_context.h
+  rst/fixes/time.cpp
   rst/fixes.cpp
   rst/fixes.h
   rst/link.cpp

source/game/actor.h

@@ -20,13 +20,17 @@ enum class Id : u16 {
   ObjRailLift = 0xd8,
   // [9] Twinmold (Red/Blue)
   BossTwinmold = 0xcc,
+  // [4] Kafei
+  NpcKafei = 0x00F4,
   // Ice platform created using ice arrows.
   BgIcePlatform = 0x013E,
+  // [4] Old Lady from Bomb Shop
+  NpcOldLady = 0x01C5,
   // [4] Rosa Sisters
   NpcRosaSisters = 0x020A,
   // [4] Bombers
   NpcBombers = 0x020F,
-  // [6] New in MM3D. Actor that shows up as sparkles and spawns an ice platform (actor 0x13E) when hit.
+  // [6] New in MM3D. Shows up as sparkles and spawns an ice platform (actor 0x13E) when hit.
   BgSeaFreezablePoint = 0x0244,
 };
 

source/game/common_data.h

@@ -107,11 +107,7 @@ struct SaveData {
   /// In-game day
   int day;
   int total_day;
-  /// Legacy time speed (?)
-  ///
-  /// Strangely enough, this is still set to -3 on the title screen. But setting this
-  /// during gameplay breaks the game as it tries to dereference a null pointer.
-  s16 legacy_time_speed, legacy_time_speed_padding;
+  int cutscene_stuff;
   /// In-game time.
   /// 0x0000 is midnight, 0x4000 is 6am, 0x8000 is noon, 0xc000 is 6pm.
   u16 time;

source/game/context.h

@@ -76,6 +76,10 @@ struct GlobalContext {
   T* FindActorWithId(act::Id id, act::Type type) const {
     return static_cast<T*>(FindActorWithId(id, type));
   }
+  bool IsActorVisible(act::Id id, act::Type type) const {
+    const auto* actor = FindActorWithId(id, type);
+    return actor && actor->draw_fn;
+  }
 
   act::Player* GetPlayerActor() const;
 

source/rst/fixes.cpp

@@ -8,31 +8,11 @@
 #include "game/actor.h"
 #include "game/actors/boss_twinmold.h"
 #include "game/actors/npc_bombers.h"
-#include "game/common_data.h"
 #include "game/context.h"
+#include "game/player.h"
 
 namespace rst {
 
-void FixTime() {
-  game::CommonData& cdata = game::GetCommonData();
-
-  // Restore the effectiveness of the Inverted Song of Time.
-  //
-  // In MM, the normal time speed is +3 and the ISoT sets the extra time speed to -2, resulting
-  // in a +1 effective time speed (which means 1/3 time speed).
-  //
-  // In MM3D, the normal speed is +2 and the ISoT only sets the extra speed to -1, which still
-  // gives the player a +1 effective speed, but only 1/2 time speed.
-  //
-  // A quick fix is to skip incrementing the in-game time every 6 frames,
-  // giving us the desired ratio of 2/6.
-
-  if (cdata.save.extra_time_speed == -2)
-    cdata.save.extra_time_speed = -1;
-  if (cdata.save.extra_time_speed == -1 && GetContext().gctx->frame_counter % 6 == 0)
-    cdata.save.extra_time_speed = -2;
-}
-
 struct TwinmoldFixState {
   s8 blue_prev_life;
   s8 red_prev_life;

source/rst/fixes.h

@@ -2,8 +2,6 @@
 
 namespace rst {
 
-void FixTime();
-
 void FixTwinmold();
 
 void FixIceArrows();

source/rst/fixes/time.cpp

@@ -0,0 +1,144 @@
+#include "rst/fixes/time.h"
+
+#include <unordered_map>
+
+#include "common/context.h"
+#include "common/debug.h"
+#include "game/common_data.h"
+#include "game/context.h"
+#include "game/static_context.h"
+
+namespace rst {
+
+static constexpr u16 HhmmToTime(int hours, int minutes, int seconds = 0) {
+  return 0x10000 * (3600.0 * hours + 60.0 * minutes + seconds) / (60 * 60 * 24);
+}
+
+static bool ShouldSlowTime() {
+  const game::GlobalContext& gctx = *GetContext().gctx;
+  const game::CommonData& cdata = game::GetCommonData();
+
+  // Require using the Inverted Song of Time
+  if (cdata.save.extra_time_speed != -1)
+    return false;
+
+  // Don't slow time to 1/3 speed if the Old Lady is in North Clock Town.
+  // The cutscene is already slow enough on 1/2 speed!
+  if ((cdata.sub1.entrance & 0xff00) == 0xD600 &&
+      gctx.FindActorWithId(game::act::Id::NpcOldLady, game::act::Type::Npc)) {
+    return false;
+  }
+
+  // Romani Ranch, alien shooting game
+  if ((cdata.sub1.entrance & 0xff00) == 0x6400 && cdata.save.day == 1 &&
+      HhmmToTime(2, 0) <= cdata.save.time && cdata.save.time <= HhmmToTime(5, 15)) {
+    return false;
+  }
+
+  // Kafei
+  if (gctx.IsActorVisible(game::act::Id::NpcKafei, game::act::Type::Npc)) {
+    return false;
+  }
+
+  return true;
+}
+
+struct NpcSchedulePosition {
+  float last_value = 0.0;
+  int ticks_since_last_update = 0;
+};
+
+static auto& GetNpcSchedulePositions() {
+  static std::unordered_map<float*, NpcSchedulePosition> s_positions;
+  return s_positions;
+}
+
+float MoveScheduledNpcHook(float* schedule_position, float speed) {
+  auto& entry = GetNpcSchedulePositions()[schedule_position];
+  if (entry.last_value < *schedule_position || *schedule_position == 0.0)
+    entry.last_value = *schedule_position;
+  entry.ticks_since_last_update = 0;
+
+  const game::CommonData& cdata = game::GetCommonData();
+  const game::StaticContext& sctx = game::GetStaticContext();
+
+  // Ensure scheduled NPCs don't go too fast.
+  //
+  // Most NPC's walking speeds are only based on the time speed which are both integers,
+  // so we need to apply the slow time multiplier manually.
+  //
+  // Using a PWM style trick for the time speed does not work well and causes
+  // flickering NPCs as they switch between different walking speeds too quickly.
+  const bool uses_custom_speed = (sctx.time_speed + cdata.save.extra_time_speed) != speed;
+  constexpr float SlowTimeMultiplier = 1.0 / 3.0;
+  return (!uses_custom_speed && ShouldSlowTime()) ? (sctx.time_speed * SlowTimeMultiplier) : speed;
+}
+
+void FixTime() {
+  for (auto it = GetNpcSchedulePositions().begin(); it != GetNpcSchedulePositions().end();) {
+    auto& [ptr, entry] = *it;
+
+    if (entry.ticks_since_last_update >= 60) {
+      // If an entry has received no update for 60 ticks, stop keeping track of it.
+      // The actor instance was probably freed.
+      // Otherwise, assume that the pointer still points to actual NPC instance data
+      // and hope we don't overwrite anything important.
+      // Yes, this is absolutely horrible.
+      util::Print("%s: forgetting %p", __func__, ptr);
+      it = GetNpcSchedulePositions().erase(it);
+      continue;
+    }
+
+    // For some reason, Nintendo/Grezzo truncates the schedule position variable
+    // every time it is supposed to stop updating (e.g. when time is frozen
+    // for the postman actor).
+    //
+    // This would normally not be an issue -- as the variable only stores integer values --
+    // but causes NPCs to turn around and walk in the wrong direction every time a loading zone
+    // is entered when the fixed Inverted Song of Time is active, as that requires using
+    // non-integer speed values.
+    //
+    // The fix is to try to detect position truncation and restore the previous value.
+    const bool was_truncated = *ptr != entry.last_value && *ptr == int(entry.last_value);
+    if (was_truncated) {
+      util::Print("%s: detected truncation for %p, restoring %f", __func__, ptr, entry.last_value);
+      *ptr = entry.last_value;
+    }
+
+    ++entry.ticks_since_last_update;
+    ++it;
+  }
+}
+
+void UpdateTimeHook() {
+  game::CommonData& cdata = game::GetCommonData();
+
+  // Restore the effectiveness of the Inverted Song of Time.
+  //
+  // In MM, the normal time speed is +3 and the ISoT sets the extra time speed to -2, resulting
+  // in a +1 effective time speed (which means 1/3 time speed).
+  //
+  // In MM3D, the normal speed is +2 and the ISoT only sets the extra speed to -1, which still
+  // gives the player a +1 effective speed, but only 1/2 time speed.
+  //
+  // A quick fix is to decrement the in-game time every 3 frames,
+  // giving us the desired ratio of 1/3 = (1+1)/(2+2+2).
+
+  if (ShouldSlowTime() && GetContext().gctx->frame_counter % 3 == 0) {
+    cdata.save.time -= 1;
+  }
+  cdata.time_copy = cdata.save.time;
+}
+
+}  // namespace rst
+
+extern "C" {
+RST_HOOK float rst_MoveScheduledNpcHook(u32, float*, float* schedule_position, u32, u32, u32*,
+                                        Vec3*, u32, float speed) {
+  return rst::MoveScheduledNpcHook(schedule_position, speed);
+}
+
+RST_HOOK void rst_UpdateTimeHook() {
+  rst::UpdateTimeHook();
+}
+}

source/rst/fixes/time.h

@@ -0,0 +1,7 @@
+#pragma once
+
+namespace rst {
+
+void FixTime();
+
+}  // namespace rst

source/rst/main.cpp

@@ -4,6 +4,7 @@
 #include "common/utils.h"
 #include "game/context.h"
 #include "rst/fixes.h"
+#include "rst/fixes/time.h"
 #include "rst/link.h"
 
 namespace rst {
@@ -38,7 +39,7 @@ void Calc(game::GlobalContext* gctx) {
 
   link::HandleFastTransform();
   link::HandleFastOcarina();
-  // FixTime();
+  FixTime();
   FixTwinmold();
   FixIceArrows();
   FixDekuMovingPlatforms();
@@ -71,7 +72,7 @@ extern void (*__init_array_end[])(void) __attribute__((weak));
 
 RST_HOOK void _start(void) {
   // Just in case something needs to be dynamically allocated...
-  static char s_fake_heap[0x10000];
+  static char s_fake_heap[0x80000];
   fake_heap_start = &s_fake_heap[0];
   fake_heap_end = &s_fake_heap[sizeof(s_fake_heap)];
 

source/rst/trampolines.s

@@ -19,3 +19,16 @@ rst_dummy:
 
 TRAMPOLINE_R0_RESULT rst_link_ShouldUseZoraFastSwim
 
+.global rst_trampoline_rst_MoveScheduledNpcHook
+.type rst_trampoline_rst_MoveScheduledNpcHook, %function
+.align 4
+rst_trampoline_rst_MoveScheduledNpcHook:
+  mov r4, r1 @ original instruction
+  push {r0-r12, lr}
+  vpush {s1-s15}
+  vpush {s16-s31}
+  # returns new speed in s0
+  bl rst_MoveScheduledNpcHook
+  vpop {s16-s31}
+  vpop {s1-s15}
+  pop {r0-r12, pc}

v100/hooks.hks

@@ -85,6 +85,17 @@ captain_keeta_speed:
   data: 00 00 60 40 # 3.5 (was 2.3333)
   addr: 0x3052C4
 
+move_scheduled_npc_hook:
+  type: branch
+  link: true
+  func: rst_trampoline_rst_MoveScheduledNpcHook
+  addr: 0x3168FC
+update_time_hook:
+  type: softbranch
+  opcode: pre
+  func: rst_UpdateTimeHook
+  addr: 0x172A64
+
 main_hook:
   type: softbranch
   opcode: post