cleanup map_object.c #306

red031000 · 2024-02-27T01:42:45Z

No description provided.

red031000 · 2024-05-10T17:21:44Z

marking as ready cause this PR is getting too big and overwhelming, expect a part 2 sometime in the future

asm/include/field_take_photo.inc

adrienntindall · 2024-05-12T15:58:59Z

asm/include/overlay_01_021F1348.inc

@@ -28,7 +28,7 @@
 .public GF3dGfxRawResObj_GetTex4x4Key
 .public GF3dGfxRawResObj_GetPlttKey
 .public sub_02026E18
-.public MapObject_GetFieldSysPtr
+.public MapObject_GetFieldSystemPtr


can drop Ptr

adrienntindall · 2024-05-12T16:00:40Z

include/fieldmap.h

@@ -34,7 +34,7 @@ u16* GetVarPointer(FieldSystem* fieldSystem, u16 varId);
 u16 FieldSystem_VarGet(FieldSystem* fieldSystem, u16 varId);
 BOOL FieldSystem_VarSet(FieldSystem* fieldSystem, u16 varId, u16 value);
 u16 FieldSystem_VarGetObjectEventGraphicsId(FieldSystem* fieldSystem, u16 objId);
-BOOL FieldSystem_FlagGet(FieldSystem *fieldSystem, u16 flagId);
+BOOL FieldSystem_FlagCheck(FieldSystem *fieldSystem, u16 flagId);


verb should come before? I remember having a discussion about this but that might've just been for the battle commands

out of scope (will fix it when I get to fieldmap)

adrienntindall · 2024-05-12T16:01:24Z

include/map_object.h

-    u32 unk0;
-    u32 unk4;
+    u32 flags;
+    u32 flags2;


any ay to distinguish between flags and flags2?

honestly I forgot I even renamed these in this ptr, I'll check when I have more time lmao

adrienntindall · 2024-05-12T16:04:48Z

src/field/overlay_01_apricorn_tree_anim.c

    GF_ASSERT(obj != NULL);

    MapObject_SetID(obj, obj_apricorn);
    MapObject_SetType(obj, 0);
    MapObject_SetFlagID(obj, 0);
    MapObject_SetXRange(obj, -1);
    MapObject_SetYRange(obj, -1);
-    MapObject_SetFlagsBits(obj, MAPOBJECTFLAG_UNK13 | MAPOBJECTFLAG_UNK10);
-    MapObject_ClearFlagsBits(obj, MAPOBJECTFLAG_UNK8 | MAPOBJECTFLAG_UNK7);
+    MapObject_SetFlagsBits(obj, (MapObjectFlagBits)(MAPOBJECTFLAG_UNK13 | MAPOBJECTFLAG_UNK10));


is it really worth having the enum if you have to keep casting it? #define could work just as well

adrienntindall · 2024-05-12T16:08:39Z

src/map_object.c

+void MapObject_Delete(LocalMapObject *object) {
+    u32 flagId = MapObject_GetFlagID(object);
+    FieldSystem *fieldSystem = MapObject_GetFieldSystemPtr(object);
+    FieldSystem_FlagSet(fieldSystem, (u16)flagId);


does this match if you change the function signature to (FieldSystem *, u16) or change flagId to a u16?

adrienntindall · 2024-05-12T16:09:42Z

src/map_object.c

+
+static void sub_0205E934(LocalMapObject *object) {
+    MapObject_SetFlagsBits(object, (MapObjectFlagBits)(MAPOBJECTFLAG_UNK2 | MAPOBJECTFLAG_ACTIVE));
+    MapObject_ClearFlagsBits(object, (MapObjectFlagBits)(MAPOBJECTFLAG_IGNORE_HEIGHTS | MAPOBJECTFLAG_UNK22 |


not sure how much I like the stacking compared to before

This should not be an enum type.

it does kinda make sense to be an enum type, it's just flags enums are annoying, I wonder if there's a way to get the compiler to work properly with this, would be good

src/map_object.c

src/unk_02055418.c

tgsm · 2024-04-20T05:26:16Z

src/encounter.c

@@ -37,7 +37,7 @@
 #include "unk_020552A4.h"
 #include "unk_02034354.h"
 #include "unk_02066EDC.h"
-#include "field_map_object.h"
+#include "map_object.h"


This is included twice

tgsm · 2024-04-20T05:32:08Z

include/map_object.h

@@ -117,6 +115,7 @@ typedef struct UnkLMOCallbackStruct2 {
 } UnkLMOCallbackStruct2;

 typedef enum MapObjectFlagBits {
+    MAPOBJECTFLAG_INACTIVE = 0,


Suggested change

MAPOBJECTFLAG_INACTIVE = 0,

MAPOBJECTFLAG_NONE = 0,

tgsm · 2024-05-13T22:46:13Z

src/map_object.c

-    u32 object_gfx_id = MapObject_GetGfxID(object);
-    if (object_gfx_id != gfx_id) {


tgsm · 2024-05-13T22:48:32Z

src/map_object.c

-    UnkLMOCallbackStruct2* unk = (gfx_id == 0x2000) ? (UnkLMOCallbackStruct2*)&ov01_0220724C : sub_0205FB38(gfx_id);
+static void sub_0205ED18(LocalMapObject *object) {
+    u32 spriteId = MapObject_GetSpriteID(object);
+    UnkLMOCallbackStruct2 *unk = (spriteId == SPRITE_CAMERA_FOCUS) ? (UnkLMOCallbackStruct2 *)&ov01_0220724C : sub_0205FB38(spriteId);


This should probably be a const pointer

PikalaxALT · 2024-05-22T00:28:34Z

include/follow_mon.h

@@ -1,7 +1,7 @@
 #ifndef POKEHEARTGOLD_FOLLOW_MON_H
 #define POKEHEARTGOLD_FOLLOW_MON_H

-#include "field_map_object.h"
+#include "map_object.h"


The original filename is adapted from gen 3, and imo equivalent routines should have equivalent (or close enough) names to facilitate transition from gen 3 to gen 4.

field does have a special meaning in gen 4 (the field overlay) which ofc is not present in gen 3, I believe this is sufficient to differentiate it

plus I genuinely don't look at gen 3 when decomping/cleaning up, the structures, consoles, and general differences are enough to make gen 3 at best misleading, at worst completely breaking for the process

PikalaxALT · 2024-05-22T00:31:49Z

src/map_object.c

-void sub_0205E104(MapObjectManager* manager, u32 a1, u32 a2, u32 num_object_events, ObjectEvent* object_events) {
-    u32 count = MapObjectManager_GetCount(manager);
-    LocalMapObject* objects = MapObjectManager_GetObjects(manager);
+extern void ov01_021F9FB0(MapObjectManager *manager, void *);


Put this in a header

PikalaxALT · 2024-05-22T00:33:31Z

src/map_object.c

+
+static void sub_0205E934(LocalMapObject *object) {
+    MapObject_SetFlagsBits(object, (MapObjectFlagBits)(MAPOBJECTFLAG_UNK2 | MAPOBJECTFLAG_ACTIVE));
+    MapObject_ClearFlagsBits(object, (MapObjectFlagBits)(MAPOBJECTFLAG_IGNORE_HEIGHTS | MAPOBJECTFLAG_UNK22 |


This should not be an enum type.

PikalaxALT · 2024-05-22T00:34:24Z

src/map_object.c

-void sub_0205EF6C(LocalMapObject* object) {
-    // No-op
+static void MapObject_NoOp(LocalMapObject *object) {
+    // NoOp


Why change this comment?

consistency, noop is NoOp elsewhere in code, and the comment should be the same as the name of the func
basically there's little to no purpose to it other than the look nice

red031000 added 8 commits April 4, 2024 01:10

cleanup map_object.c 1

616b12e

dumbass

4fec22c

transferring to another computer

7988706

some more

93073fe

more

b8c03fd

*screams*

26c2b1e

more

fc2e957

a

f91a7dc

red031000 force-pushed the master branch from ce3a54a to f91a7dc Compare April 3, 2024 23:11

red031000 added 11 commits April 4, 2024 18:43

more work

794b302

forgot a static

3e1e669

more cleanup

dd8c551

more work

144ee05

I'm really sorry whoever reviews this

507691a

a bit more

75a7f0d

Merge branch 'master' of https://github.com/pret/pokeheartgold

233cf96

moree

3682dcd

forgot to commit this

2ce20fa

more

dbe94bf

Merge branch 'master' into master

4bff6f9

red031000 marked this pull request as ready for review May 10, 2024 17:21

adrienntindall reviewed May 12, 2024

View reviewed changes

asm/include/field_take_photo.inc Show resolved Hide resolved

adrienntindall requested changes May 12, 2024

View reviewed changes

tgsm requested changes May 13, 2024

View reviewed changes

PikalaxALT requested changes May 22, 2024

View reviewed changes

PikalaxALT added the CLEANUP label May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup map_object.c #306

cleanup map_object.c #306

red031000 commented Feb 27, 2024

red031000 commented May 10, 2024

adrienntindall May 12, 2024

adrienntindall May 12, 2024

red031000 May 23, 2024

adrienntindall May 12, 2024

red031000 May 13, 2024

adrienntindall May 12, 2024

adrienntindall May 12, 2024

adrienntindall May 12, 2024

PikalaxALT May 22, 2024

red031000 May 23, 2024

tgsm Apr 20, 2024

tgsm Apr 20, 2024

tgsm May 13, 2024

tgsm May 13, 2024

PikalaxALT May 22, 2024

red031000 May 23, 2024

PikalaxALT May 22, 2024

PikalaxALT May 22, 2024

PikalaxALT May 22, 2024

red031000 May 23, 2024

		u32 object_gfx_id = MapObject_GetGfxID(object);
		if (object_gfx_id != gfx_id) {

cleanup map_object.c #306

Are you sure you want to change the base?

cleanup map_object.c #306

Conversation

red031000 commented Feb 27, 2024

red031000 commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment