Miscellaneous labels #153

Sha0den · 2024-09-08T00:21:39Z

I tried to include most (if not all) of the label changes from my poketcg_v2 repository. I also identified some duel constants.

ElectroDeoxys

Overall great stuff! Only one thing I've nitpicked

ElectroDeoxys · 2024-09-11T14:24:51Z

src/home/duel.asm

@@ -1692,7 +1692,7 @@ SendAttackDataToLinkOpponent::
 	ldh [hTemp_ffa0], a
 	ret

-Func_189d::
+LastChanceToNegateFinalDamage::


This name doesn't sit right with me, at its core this routine only relates to Transparency, and whether to apply it. My suggestion would be ApplyTransparencyIsApplicable or something along those lines. What do you think?

That works for me. I believe I took that label name from Oatspear's hack, and while it makes a lot of sense from a romhacking perspective, I can definitely see that yours is a more accurate description of the current function. While I'm at it, I think I'll also change DeckShuffleAnimation to PlayDeckShuffleAnimation.

Sorry, I meant ApplyTransparencyIfApplicable

That's mostly on me. I thought it sounded weird, but I still copied and pasted what you typed without asking for a clarification. Anyway, it's fixed now.

ElectroDeoxys

I don't know how I feel about generalising the names on some routines, for example in this game only Headache can stop Trainer cards from being used. But then there's an argument to be made for consistency with the sequel.

ElectroDeoxys · 2024-09-16T23:01:27Z

src/home/substatus.asm

+; returns carry if Pokemon in turn holder's Play Area location in register a
+; cannot use its Pkmn Power
+; input:
+;	a = play area location offset of the Pokémon to check (PLAY_AREA_* constant)


This description is not correct, if register a is non-zero, then status is ignored and only Toxic Gas is considered. It does not have anything to do with Play Area locations. But the current label is also incomplete since it checks for both status and Toxic Gas. Maybe CheckIsIncapableOfUsingPkmnPower? And the routine above may be CheckIsIncapableOfUsingPkmnPower_IgnoreStatus?

I'm forced to disagree. In almost every instance, a play area location offset gets loaded into the a register before this function is called, even if the specifics of this function only care about whether or not the location is the Arena (because an Active Pokémon's Pokémon Power cannot be used if it is Asleep, Confused, or Paralyzed). I think the labels from poketcg2 are about as accurate as you can get. Plus, your CheckIsIncapableOfUsingPkmnPower_IgnoreStatus label would imply the opposite of what that first function does, which is to set a to 0 (PLAY_AREA_ARENA) so that the Active Pokémon's status gets checked. The only instance of the game intentionally ignoring the status check is when a Pokémon with a Pokémon Power is put onto the Bench; in that singular instance, $01 (PLAY_AREA_BENCH1) is loaded into a before this function is called.

Ah well spotted, now that I look at it it does make sense

Sha0den · 2024-09-17T03:59:02Z

I don't know how I feel about generalising the names on some routines, for example in this game only Headache can stop Trainer cards from being used. But then there's an argument to be made for consistency with the sequel.

For the most part, I agree with what you said about over generalizing label names, but in this particular instance, I feel that the pros outweigh the cons. Using an effect to stop Trainer cards from being played or an Active Pokémon from being able to retreat is fairly common within the Pokémon TCG at large, but the original attacks (Headache and Acid) that are used in this game are somewhat obscure, so a more general label will do a better job of letting the reader know the function's purpose. And as you pointed out, having consistent label names between the 2 disassembly projects would be ideal.

dannye · 2024-09-24T02:23:18Z

src/engine/menus/config.asm

@@ -5,7 +5,7 @@ _PauseMenu_Config:
 	push af
 	xor a
 	ld [wConfigExitSettingsCursorPos], a
-	ld a, 1
+	ld a, $01 ; text isn't double-spaced


Rather than changing this instance to $01, I would rather change most other instances to 1. Or use a constant. It would be tempting to use TRUE but that would feel inverted from the real meaning based on the current name of wLineSeparation. If it were named like eg wSingleSpaced then TRUE would feel more natural. Or we could leave the name alone and make a special SINGLE_SPACED constant instead of using TRUE. Just rambling..

I only added those "double-spaced" comments to try to make the updates to that variable easier to understand. I also agree that either of your suggestions would probably be a better idea than my comments. Just tell me what you would prefer, and I'll update the pull request.

My vote would probably be for adding:

DEF DOUBLE_SPACED EQU 0 DEF SINGLE_SPACED EQU 1

and then doing a bunch of:

- xor a ; text is double-spaced + xor a ; DOUBLE_SPACED

and

- ld a, $01 ; text isn't double-spaced + ld a, SINGLE_SPACED

src/home/substatus.asm

dannye · 2024-09-24T02:30:02Z

src/engine/duel/effect_functions.asm

@@ -98,14 +98,14 @@ TossCoinATimes_BankB:
 CommentedOut_2c086:
 	ret

-Func_2c087:
+Serial_TossZeroCoins:


dannye · 2024-09-24T02:34:48Z

src/engine/duel/ai/trainer_cards.asm

@@ -3643,7 +3644,7 @@ PickPokedexCards_Unreferenced:
 ; stores the resulting order in wce1a.
 PickPokedexCards:
 	xor a
-	ld [wAIPokedexCounter], a ; reset counter ; reset counter


dannye · 2024-09-24T02:42:54Z

src/engine/duel/ai/core.asm

+; input:
+;	hl = address from which to start copying the data
+;	de = where to copy the data
+CopyListWithFFTerminatorFromHLToDE_Bank5:


I feel like this is kind of a mouthful compared to the previous name. IMO we could exclude "WithFFTerminator" from the name.

That one's tricky. I only added the "WithFFTerminator" because there's already a CopyListFromHLToDE in src/engine/menus/deck_configuration.asm (Bank $02), and that function requires a null-terminated list. If you have any suggestions now that you know the problem, I'm all ears.

Hm, good point. It's okay as-is.

If we really wanted to give them better names, I would maybe suggest renaming CopyListFromHLToDE to CopyStringFromHLToDEsince strings are typically null-terminated, then renaming CopyListWithFFTerminatorFromHLToDE to CopyListFromHLToDE since lists are typically ff-terminated. But I'm not totally convinced it's worth it.

src/engine/duel/ai/common.asm

dannye

Looks good to me, thanks for taking the time to do this. I'll give @ElectroDeoxys a chance to take one more look.

Sha0den added 3 commits September 7, 2024 16:40

Identify some more variables using existing constants

7889bf9

Matching labels for identical AI Logic functions

94858fe

Label various unlabeled functions

f9a8aac

ElectroDeoxys reviewed Sep 11, 2024

View reviewed changes

Sha0den added 3 commits September 11, 2024 14:23

Adjust 2 of the new labels

cd3867a

Fix Label Typo

68f981d

Borrow some substatus function labels from poketcg2 dissassembly

fb2b749

ElectroDeoxys requested changes Sep 16, 2024

View reviewed changes

dannye reviewed Sep 24, 2024

View reviewed changes

Sha0den added 3 commits September 24, 2024 02:34

Revert -1 -> $ff changes in substatus.asm

915e498

Use ClearMemory_Bank2 instead of ClearNBytesFromHL

0285528

Add wLineSeparation constants

d1dd98c

dannye approved these changes Sep 24, 2024

View reviewed changes

dannye merged commit e4cf3fc into pret:master Oct 3, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous labels #153

Miscellaneous labels #153

Sha0den commented Sep 8, 2024

ElectroDeoxys left a comment

ElectroDeoxys Sep 11, 2024

Sha0den Sep 11, 2024

ElectroDeoxys Sep 13, 2024

Sha0den Sep 13, 2024

ElectroDeoxys left a comment

ElectroDeoxys Sep 16, 2024

Sha0den Sep 17, 2024

ElectroDeoxys Sep 19, 2024

Sha0den commented Sep 17, 2024

dannye Sep 24, 2024

Sha0den Sep 24, 2024

dannye Sep 24, 2024

dannye Sep 24, 2024

dannye Sep 24, 2024

dannye Sep 24, 2024

Sha0den Sep 24, 2024

dannye Sep 24, 2024

dannye left a comment

Miscellaneous labels #153

Miscellaneous labels #153

Conversation

Sha0den commented Sep 8, 2024

ElectroDeoxys 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

ElectroDeoxys 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

Sha0den commented Sep 17, 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

dannye left a comment

Choose a reason for hiding this comment