mystery_gift.c #271

PikalaxALT · 2023-12-06T02:14:25Z

No description provided.

adrienntindall

First pass while on mobile on the bus

include/mystery_gift.h

adrienntindall · 2023-12-07T19:00:59Z

src/mystery_gift.c

+
+BOOL SaveMysteryGift_ReceiveGiftAndClearCardByIndex(MYSTERY_GIFT_SAVE* mg, int index) {
+    GF_ASSERT(index < NUM_SAVED_WONDER_CARDS);
+    mg->cards[index].tag = MG_TAG_invalid;


MG_TAG_INVALID?

Would need to change all the other constants in this define list.

src/mystery_gift.c

red031000 · 2023-12-11T07:59:27Z

include/mystery_gift.h

+    u8 raw[256];
+} MysteryGiftData;
+
+typedef struct {


struct name here

red031000 · 2023-12-11T07:59:59Z

include/mystery_gift.h

+// will return a non-nullable pointer to a
+// special Wonder Card slot that's exclusive
+// to HGSS. Otherwise, returns NULL.
+WonderCard* SaveMysteryGift_CardGetByIdx(MysteryGiftSave* mg, int index);


change all these to Save_MysteryGift

red031000 · 2023-12-11T08:00:24Z

src/mystery_gift.c

+#include "save_arrays.h"
+#include "constants/save_arrays.h"
+
+BOOL MysteryGiftTagIsValid(u32 tag);


MysteryGift_TagIsValid

This name implies the argument is a MysteryGift*, which it isn't.

no? the name implies that the function is to do with mystery gifts, I thought that was how the naming is done, not based on arguments

The nomenclature is based on the pseudo-OOP. MysteryGift_TagIsValid would be read as if it was the C++ routine bool MysteryGift::TagIsValid() const.

I feel like further discussion is needed on what the prefixes are based on exactly, so it's down and agreed

Pika is right about what we based this nomenclature on.

red031000 · 2023-12-11T08:00:38Z

src/mystery_gift.c

+#include "constants/save_arrays.h"
+
+BOOL MysteryGiftTagIsValid(u32 tag);
+MysteryGift* SaveMysteryGift_GetByIdx(MysteryGiftSave* mg, int index);


Index instead of Idx

What's the real advantage of spelling out "index" here? "Idx" is universally understood to mean "Index".

style agreed idx bad

reddo: "I consent to style"
donnel: "I consent to style"
pika: "I don't"

is there someone you forgot to ask?

Idgaf as long as it's readable, Idx is still better than I

red031000 · 2023-12-11T08:01:17Z

src/mystery_gift.c

+
+BOOL MysteryGiftTagIsValid(u32 tag);
+MysteryGift* SaveMysteryGift_GetByIdx(MysteryGiftSave* mg, int index);
+BOOL SaveMysteryGift_SetReceivedByIdx(MysteryGiftSave* mg, int index);


pointer alignment on all these

Not sure what you mean here

pointer should be MysteryGiftSave *mg instead of current

no, that is ugly af

that's agreed by several people, and is the style in the whole repo

it's ok red we can just fix it if pika wants to be dense here

red031000 · 2023-12-11T08:01:29Z

include/mystery_gift.h

+
+// Returns TRUE if there is an open slot
+// to receive a gift. The capacity is 8.
+BOOL SaveMysteryGift_FindAvailable(const MysteryGiftSave* mg);


pointer alignment

src/mystery_gift.c

red031000 · 2023-12-14T15:49:23Z

still a bit of issue with style here, I would like it to be followed because it has been agreed upon (and it was quite difficult to find something that most, if not everyone agreed with)

adrienntindall · 2023-12-15T07:14:46Z

include/mystery_gift.h

+    u32 item;
+    u16 ruleset[24];
+    int base_decoration;
+    MG_MON_DECO_TAG mon_decoration;


Don't use snake_case for struct variables

Also MG_MON_DECO_TAG to non-capitalized enum/struct name

tbf this used to be an anonymous union inside struct MysteryGift. variant names are unchanged from that union.

adrienntindall · 2023-12-15T07:16:47Z

include/mystery_gift.h

+    int base_decoration;
+    MG_MON_DECO_TAG mon_decoration;
+    u8 pokewalkerCourse;
+    PHOTO photo;


PHOTO -> Photo

Out of scope

adrienntindall · 2023-12-15T07:18:18Z

src/mystery_gift.c

+#include "save_arrays.h"
+#include "constants/save_arrays.h"
+
+BOOL MysteryGiftTagIsValid(u32 tag);


Pika is right about what we based this nomenclature on.

adrienntindall · 2023-12-15T07:18:43Z

src/mystery_gift.c

+
+BOOL MysteryGiftTagIsValid(u32 tag);
+MysteryGift* SaveMysteryGift_GetByIdx(MysteryGiftSave* mg, int index);
+BOOL SaveMysteryGift_SetReceivedByIdx(MysteryGiftSave* mg, int index);


it's ok red we can just fix it if pika wants to be dense here

adrienntindall · 2023-12-15T07:21:57Z

src/mystery_gift.c

+    }
+}
+
+int GetFirstQueuedMysteryGiftIdx(void) {


Arguably using the SaveMysteryGift(Data) prefix here is fine since it's modifying the static instance of SaveMysteryGiftData, ie analogous to a static function of the class. Ditto for everything else.

In my scrcmd_11 PR, I explore the possibility of this static pointer, at one point in development, holding a transaction. Upon further study of retsam, this was not the case, but wouldn't it be cool if it was?

adrienntindall · 2023-12-15T07:23:20Z

src/mystery_gift.c

+
+static MysteryGiftSave* sMysteryGiftSaveData;
+
+void GetStaticPointerToSaveMysteryGift(SaveData* saveData) {


Related to the comment below - GetStaticPointerToSaveMysteryGift is a whole hell of a lot worse than MysteryGiftSave_SetInstance/_SetFromSave or something similar. I think

red031000

honestly at this point I feel like I'll just go through and correct the style myself when I get to that file, I don't want to block this for much longer

PikalaxALT added 3 commits December 1, 2023 19:06

Through sub_0202DC2C

ec1f0aa

Finish mystery_gift.c

f8c138d

Rename mystery_gift.c functions

2ffd8f2

PikalaxALT marked this pull request as ready for review December 7, 2023 01:37

PikalaxALT requested review from red031000 and adrienntindall December 7, 2023 01:37

PikalaxALT added 2 commits December 6, 2023 21:08

Update fn names; document public routines

ac98817

capitalize MG_TAG_* defines

6bd2b0a

adrienntindall requested changes Dec 7, 2023

View reviewed changes

PikalaxALT mentioned this pull request Dec 9, 2023

Mystery Gift script commands #273

Merged

Merge branch 'master' into mystery_gift

fdbb0d4

tgsm requested changes Dec 11, 2023

View reviewed changes

src/mystery_gift.c Outdated Show resolved Hide resolved

src/mystery_gift.c Outdated Show resolved Hide resolved

src/mystery_gift.c Outdated Show resolved Hide resolved

red031000 requested changes Dec 11, 2023

View reviewed changes

Update some names; simplify some statements

4c6aa45

adrienntindall requested changes Dec 15, 2023

View reviewed changes

Address other review comments

7334c8e

PikalaxALT requested review from adrienntindall and tgsm December 17, 2023 01:51

Merge remote-tracking branch 'origin/master' into mystery_gift

1562b47

tgsm approved these changes Dec 18, 2023

View reviewed changes

red031000 approved these changes Dec 18, 2023

View reviewed changes

Merge branch 'master' into mystery_gift

1338864

adrienntindall approved these changes Dec 19, 2023

View reviewed changes

adrienntindall merged commit 439efca into pret:master Dec 19, 2023
1 check passed

github-actions bot pushed a commit that referenced this pull request Dec 19, 2023

Merge pull request #271 from PikalaxALT/mystery_gift

03a9f9e


		static MysteryGiftSave* sMysteryGiftSaveData;

		void GetStaticPointerToSaveMysteryGift(SaveData* saveData) {

mystery_gift.c #271

mystery_gift.c #271

Conversation

PikalaxALT commented Dec 6, 2023

adrienntindall left a 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

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

red031000 commented Dec 14, 2023

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

red031000 left a comment

Choose a reason for hiding this comment