improve decomp of state handlers and art groups (#3014)

- state handlers that are not inlined lambdas have smarter type
checking, getting rid of 99.9% of the casts emitted (they were not
useful)
- art groups were not being properly linked to their "master" groups.
- `max` in `ja` in Jak 2 was not being detected.

Another huge PR...

common/type_system/TypeSpec.cpp

@@ -136,3 +136,12 @@ void TypeSpec::add_or_modify_tag(const std::string& tag_name, const std::string&
   }
   m_tags.push_back({tag_name, tag_value});
 }
+
+void TypeSpec::delete_tag(const std::string& tag_name) {
+  for (size_t i = 0; i < m_tags.size(); ++i) {
+    if (m_tags.at(i).name == tag_name) {
+      m_tags.erase(m_tags.begin() + i);
+      return;
+    }
+  }
+}

common/type_system/TypeSpec.h

@@ -94,6 +94,7 @@ class TypeSpec {
   const std::string& get_tag(const std::string& tag_name) const;
   void modify_tag(const std::string& tag_name, const std::string& tag_value);
   void add_or_modify_tag(const std::string& tag_name, const std::string& tag_value);
+  void delete_tag(const std::string& tag_name);
 
   const std::string& base_type() const { return m_type; }
 

common/type_system/TypeSystem.cpp

@@ -1590,7 +1590,7 @@ bool TypeSystem::typecheck_and_throw(const TypeSpec& expected,
       if (!got) {
         success = false;
       } else {
-        if (*got != tag.value) {
+        if (!tc(tag.value, *got)) {
           success = false;
         }
       }

decompiler/IR2/FormExpressionAnalysis.cpp

@@ -3881,7 +3881,7 @@ void FunctionCallElement::update_from_stack(const Env& env,
         macro.push_back(rate);
       }
 
-      if (env.version > GameVersion::Jak1) {
+      if (env.version != GameVersion::Jak1) {
         macro.push_back(pool.form<ConstantTokenElement>(":origin-is-matrix"));
         macro.push_back(pool.form<ConstantTokenElement>("#t"));
       }
@@ -3918,10 +3918,8 @@ ConstantTokenElement* DerefElement::try_as_art_const(const Env& env, FormPool& p
     if (elt_name) {
       return pool.alloc_element<ConstantTokenElement>(*elt_name);
     } else {
-      if (env.version != GameVersion::Jak2) {
-        lg::error("function {}: did not find art element {} in {}", env.func->name(),
-                  mr.maps.ints.at(0), env.art_group());
-      }
+      lg::error("function `{}`: did not find art element {} in {}", env.func->name(),
+                mr.maps.ints.at(0), env.art_group());
     }
   }
 
@@ -4841,7 +4839,6 @@ FormElement* try_make_logtest_mouse_macro(Form* in, FormPool& pool) {
       t = REL;
     }
 
-    printf("t is %d\n", t);
     if (t != NIL) {
       auto logtest_elt = dynamic_cast<GenericElement*>(in->at(0));
       if (logtest_elt) {

decompiler/ObjectFile/ObjectFileDB.cpp

@@ -878,11 +878,12 @@ void get_art_info(ObjectFileDB& db, ObjectFileData& obj) {
     const auto& words = obj.linked_data.words_by_seg.at(MAIN_SEGMENT);
     if (words.at(0).kind() == LinkedWord::Kind::TYPE_PTR &&
         words.at(0).symbol_name() == "art-group") {
+      auto obj_unique_name = obj.to_unique_name();
+
       // lg::print("art-group {}:\n", obj.to_unique_name());
       auto name = obj.linked_data.get_goal_string_by_label(words.at(2).label_id());
       int length = words.at(3).data;
       // lg::print("  length: {}\n", length);
-      std::unordered_map<int, std::string> art_group_elts;
       for (int i = 0; i < length; ++i) {
         const auto& word = words.at(8 + i);
         if (word.kind() == LinkedWord::Kind::SYM_PTR && word.symbol_name() == "#f") {
@@ -891,8 +892,19 @@ void get_art_info(ObjectFileDB& db, ObjectFileData& obj) {
         const auto& label = obj.linked_data.labels.at(word.label_id());
         auto elt_name =
             obj.linked_data.get_goal_string_by_label(words.at(label.offset / 4 + 1).label_id());
+        auto unique_name = elt_name;
+
+        auto ag_name = obj_unique_name;
+        int elt_index = i;
+        auto& word_master_ag = words.at(label.offset / 4 + 7);
+        auto& word_master_idx = words.at(label.offset / 4 + 8);
+        if (word_master_ag.kind() == LinkedWord::Kind::PTR &&
+            word_master_idx.kind() == LinkedWord::Kind::PLAIN_DATA) {
+          ag_name = obj.linked_data.get_goal_string_by_label(word_master_ag.label_id()) + "-ag";
+          elt_index = word_master_idx.data;
+        }
+
         std::string elt_type = words.at(label.offset / 4 - 1).symbol_name();
-        std::string unique_name = elt_name;
         if (elt_type == "art-joint-geo") {
           // the skeleton!
           unique_name += "-jg";
@@ -908,10 +920,9 @@ void get_art_info(ObjectFileDB& db, ObjectFileData& obj) {
           throw std::runtime_error(
               fmt::format("unknown art elt type {} in {}", elt_type, obj.to_unique_name()));
         }
-        art_group_elts[i] = unique_name;
-        // lg::print("  {}: {} ({}) -> {}\n", i, elt_name, elt_type, unique_name);
+        // lg::print("  {}: {} ({}) -> {} @ {}\n", i, elt_name, elt_type, unique_name, elt_index);
+        db.dts.add_art_group_elt(ag_name, unique_name, elt_index);
       }
-      db.dts.art_group_info[obj.to_unique_name()] = art_group_elts;
     }
   }
 }

decompiler/analysis/expression_build.cpp

@@ -111,8 +111,8 @@ bool convert_to_expressions(
 
           // the return value of this function is not an exact match
           // we cast it to avoid complicated issues with polymorphism (e.g. methods)
-          // we don't run this if we find a (return statement because we do not handle
-          // type checking on those at the moment.
+          // we don't run this if we find a (return statement for unknown reasons
+          // TODO : remove this and check?
           if (!found_early_return && f.type.last_arg() != return_type) {
             needs_cast = true;
           }

decompiler/analysis/find_defstates.cpp

@@ -94,8 +94,8 @@ std::vector<DefstateElement::Entry> get_defstate_entries(
           temp.to_string(env));
     }
 
-    auto var = mr.maps.regs.at(0);
-    auto name = mr.maps.strings.at(1);
+    auto& var = mr.maps.regs.at(0);
+    auto& name = mr.maps.strings.at(1);
     auto val = mr.maps.forms.at(2);
 
     auto handler_kind = handler_name_to_kind(name);
@@ -149,12 +149,35 @@ std::vector<DefstateElement::Entry> get_defstate_entries(
       // hack - lets pretend every handler (except event) returns none but remove the (none) at the
       // end since the 'real' return type is object and thus anything is valid in the final form
       handler_func->ir2.skip_final_none = true;
-    } else if (handler_atom && handler_atom->is_sym_val()) {
+    } else if (handler_atom && handler_atom->is_sym_val() && !handler_atom->is_sym_val("#f")) {
+      // value of a symbol.
+      // NOTE : we
       auto sym_type = env.dts->lookup_symbol_type(handler_atom->get_str());
-      auto expected_type = get_state_handler_type(handler_kind, state_type);
-      if (!env.dts->ts.tc(expected_type, sym_type)) {
+      auto handler_type = get_state_handler_type(handler_kind, state_type);
+      // NOTE : we set the return value of the handlers to "none" for a sneaky hack (see right
+      // above) however we only need that when decompiling lambdas. so we revert that hack here.
+      if (handler_type.last_arg() == TypeSpec("none")) {
+        handler_type.last_arg() = TypeSpec("object");
+      }
+      // hack : delete the behavior tags and typecheck them separately.
+      auto sym_behavior = sym_type.try_get_tag("behavior");
+      handler_type.delete_tag("behavior");
+      sym_type.delete_tag("behavior");
+
+      // finally do typecheck. does argument typecheck, then we do process typecheck separately.
+      // this is because the logic has to be kind of backwards:
+      // - the process type in the symbol's behavior tag can be more generic; but
+      // - the arguments can be more specific.
+      // for example, a process of type `enemy` can run a behavior for `process-drawable` just fine
+      // but a behavior for `process-drawable` that requires a more specific argument than `enemy`
+      // would be bad.
+      // in practice this just allows us to use more specific types for functions in the decompiler
+      // without trigger extraneous casts.
+      if (!env.dts->ts.tc(handler_type, sym_type) ||
+          (sym_behavior.has_value() &&
+           !env.dts->ts.tc(*sym_behavior, state_type.last_arg().base_type()))) {
         this_entry.val =
-            pool.alloc_single_element_form<CastElement>(nullptr, expected_type, this_entry.val);
+            pool.alloc_single_element_form<CastElement>(nullptr, handler_type, this_entry.val);
       }
     }
     // name = code/event/etc

decompiler/analysis/insert_lets.cpp

@@ -1330,14 +1330,19 @@ FormElement* rewrite_joint_macro(LetElement* in, const Env& env, FormPool& pool)
                 Matcher::any_reg(0), false,
                 {DerefTokenMatcher::string("frame-group"), DerefTokenMatcher::string("frames"),
                  DerefTokenMatcher::string("num-frames")});
+  auto matcher_new_group_max_frames =
+      env.version == GameVersion::Jak1
+          ? Matcher::deref(Matcher::any(1), false,
+                           {DerefTokenMatcher::string("data"), DerefTokenMatcher::integer(0),
+                            DerefTokenMatcher::string("length")})
+          : Matcher::deref(
+                Matcher::any(1), false,
+                {DerefTokenMatcher::string("frames"), DerefTokenMatcher::string("num-frames")});
   auto matcher_max_num = Matcher::cast(
-      "float", Matcher::op_fixed(FixedOperatorKind::ADDITION,
-                                 {form_fg ? Matcher::deref(Matcher::any(1), false,
-                                                           {DerefTokenMatcher::string("data"),
-                                                            DerefTokenMatcher::integer(0),
-                                                            DerefTokenMatcher::string("length")})
-                                          : matcher_cur_group_max_frames,
-                                  Matcher::integer(-1)}));
+      "float",
+      Matcher::op_fixed(FixedOperatorKind::ADDITION,
+                        {form_fg ? matcher_new_group_max_frames : matcher_cur_group_max_frames,
+                         Matcher::integer(-1)}));
 
   // DONE CHECKING EVERYTHING!!! Now write the goddamn macro.
   std::vector<Form*> args;

decompiler/config/jak1/all-types.gc

@@ -33724,7 +33724,7 @@
 ;; - Functions
 
 (define-extern minershort-trans-hook (function none :behavior minershort))
-(define-extern miners-anim-loop (function none :behavior minershort))
+(define-extern miners-anim-loop (function object :behavior process-taskable))
 
 ;; - Unknowns
 

decompiler/config/jak2/all-types.gc

@@ -28409,7 +28409,7 @@
 (define-extern ja-eval (function int :behavior process-drawable))
 (define-extern ja-blend-eval (function int :behavior process-drawable))
 (define-extern ja-post (function none :behavior process-drawable))
-(define-extern sleep-code (function symbol :behavior process-drawable))
+(define-extern sleep-code (function symbol :behavior process))
 (define-extern transform-and-sleep (function none :behavior process-drawable))
 (define-extern transform-and-sleep-code (function none :behavior process-drawable))
 (define-extern transform-post (function int :behavior process-drawable))
@@ -29572,7 +29572,7 @@
 (define-extern projectile-update-velocity-space-wars (function projectile none))
 (define-extern projectile-init-by-other (function projectile-init-by-other-params projectile :behavior projectile))
 (define-extern projectile-bounce-update-velocity (function projectile-bounce none :behavior projectile))
-(define-extern projectile-bounce-falling-post (function projectile-bounce none :behavior projectile-bounce))
+(define-extern projectile-bounce-falling-post (function none :behavior projectile-bounce))
 (define-extern projectile-bounce-move (function projectile-bounce none))
 (define-extern projectile-bounce-reaction (function control-info collide-query vector vector collide-status))
 

decompiler/config/jak2/ntsc_v1/art_info.jsonc

@@ -1,4 +1,29 @@
 {
-  "files": {},
-  "functions": {}
+  //////////////////////
+  // ART INFO
+  //////////////////////
+
+  // defines what art group each file or function is using.
+  // by default, the decompiler assumes to be the name of the current file + -ag
+  // so you only need to specify it when that's not the case.
+  // NOTE: it's fine to have a function and its file both in here. the function takes priority.
+
+  "files": {
+    "target": "jakb-ag",
+    "target2": "jakb-ag",
+    "target-death": "jakb-ag",
+    "target-gun": "jakb-ag",
+    "target-swim": "jakb-ag",
+    "target-tube": "jakb-ag",
+    "target-board": "jakb-ag",
+    "target-carry": "jakb-ag",
+    "target-darkjak": "jakb-ag",
+    "target-anim": "jakb-ag",
+    "powerups": "jakb-ag",
+    "sidekick": "daxter-ag"
+  },
+
+  "functions": {
+    // "(code target-warp-out)": "eichar-ag"
+  }
 }

decompiler/util/DecompilerTypeSystem.h

@@ -69,6 +69,15 @@ class DecompilerTypeSystem {
   TypeSpec lookup_symbol_type(const std::string& name) const;
   bool should_attempt_cast_simplify(const TypeSpec& expected, const TypeSpec& actual) const;
 
+  void add_art_group_elt(const std::string& ag_name, const std::string& elt_name, int elt_index) {
+    if (art_group_info.count(ag_name) == 0) {
+      art_group_info[ag_name] = {};
+    }
+    if (art_group_info.at(ag_name).count(elt_index) == 0) {
+      art_group_info.at(ag_name)[elt_index] = elt_name;
+    }
+  }
+
   // todo - totally eliminate this.
   struct {
     std::string current_method_type;

game/graphics/opengl_renderer/CollideMeshRenderer.cpp

@@ -270,7 +270,5 @@ void CollideMeshRenderer::render(SharedRenderState* render_state, ScopedProfiler
 
     prof.add_draw_call();
     prof.add_tri(lev->level->collision.vertices.size() / 3);
-
-    last_render_time = current_time;
   }
 }

game/graphics/opengl_renderer/CollideMeshRenderer.h

@@ -15,9 +15,6 @@ class CollideMeshRenderer {
   void render(SharedRenderState* render_state, ScopedProfilerNode& prof);
   ~CollideMeshRenderer();
 
-  int current_time = 0;
-  int last_render_time = 0;
-
  private:
   void init_pat_colors(GameVersion version);
 

game/graphics/opengl_renderer/OpenGLRenderer.cpp

@@ -1010,8 +1010,6 @@ void OpenGLRenderer::dispatch_buckets_jak2(DmaFollower dma,
   m_render_state.bucket_for_vis_copy = (int)jak2::BucketId::BUCKET_2;
   m_render_state.num_vis_to_copy = jak2::LEVEL_MAX;
 
-  ++m_collide_renderer.current_time;
-
   for (size_t bucket_id = 0; bucket_id < m_bucket_renderers.size(); bucket_id++) {
     auto& renderer = m_bucket_renderers[bucket_id];
     auto bucket_prof = prof.make_scoped_child(renderer->name_and_id());
@@ -1032,8 +1030,7 @@ void OpenGLRenderer::dispatch_buckets_jak2(DmaFollower dma,
 
     // hack to draw the collision mesh in the middle the drawing
     if (bucket_id + 1 == (int)jak2::BucketId::TEX_L0_ALPHA &&
-        Gfx::g_global_settings.collision_enable &&
-        m_collide_renderer.last_render_time != m_collide_renderer.current_time) {
+        Gfx::g_global_settings.collision_enable) {
       auto p = prof.make_scoped_child("collision-draw");
       m_collide_renderer.render(&m_render_state, p);
     }

goal_src/jak1/engine/anim/joint-exploder.gc

@@ -523,7 +523,7 @@
       (ja :num! (loop!))
       )
     )
-  :post (the-as (function none :behavior joint-exploder) ja-post)
+  :post ja-post
   )
 
 (defmethod joint-exploder-method-23 joint-exploder ((obj joint-exploder))

goal_src/jak1/engine/common-obs/basebutton.gc

@@ -102,7 +102,7 @@
         (rider-trans)
         )
     )
-  :code (the-as (function none :behavior basebutton) anim-loop)
+  :code anim-loop
   :post (behavior ()
     (when (-> self move-to?)
       (set! (-> self move-to?) #f)
@@ -129,7 +129,7 @@
   :enter (behavior ()
     (press! self #t)
     )
-  :trans (the-as (function none :behavior basebutton) rider-trans)
+  :trans rider-trans
   :code (behavior ()
     (ja-no-eval :num! (seek! max (-> self anim-speed)))
     (until (ja-done? 0)
@@ -212,7 +212,7 @@
   :enter (behavior ()
     (press! self #f)
     )
-  :trans (the-as (function none :behavior basebutton) rider-trans)
+  :trans rider-trans
   :code (behavior ()
     (ja-no-eval :num! (seek! 0.0 (-> self anim-speed)))
     (until (ja-done? 0)

goal_src/jak1/engine/common-obs/baseplat.gc

@@ -251,7 +251,7 @@ eco-door-event-handler
       )
     (go-virtual door-open)
     )
-  :post (the-as (function none :behavior eco-door) transform-post)
+  :post transform-post
   )
 
 (defstate door-open (eco-door)
@@ -309,7 +309,7 @@ eco-door-event-handler
         )
     (go-virtual door-closed)
     )
-  :post (the-as (function none :behavior eco-door) transform-post)
+  :post transform-post
   )
 
 (defmethod eco-door-method-26 eco-door ((obj eco-door))

goal_src/jak1/engine/common-obs/collectables.gc

@@ -391,7 +391,7 @@
         (go-virtual wait)
         )
     )
-  :code (the-as (function none :behavior eco-collectable) anim-loop)
+  :code anim-loop
   )
 
 (defstate jump (eco-collectable)