pret / pokeheartgold

Decompilation of Pokemon HeartGold/SoulSilver
306 stars 97 forks source link

Script/Bytecode styling #262

Open luckytyphlosion opened 10 months ago

luckytyphlosion commented 10 months ago

Script/Bytecode styling

Probably don't need to explain what scripts are (e.g. trainer AI scripts, battle scripts, event scripts (if they're even called that in Gen 4)).

Shorthand for options are in the format ., e.g. for snake_case local labels, write 3a.2. But it's always good to mention what the option is anyway.

Example format for what styling you want

Command name styling: PascalCase Entry-point label styling: PascalCase Use local labels: No Within-subroutine label styling: _UnderscorePrefixPascalCase

Copypasteable version:

Command name styling: `PascalCase`
Entry-point label styling: `PascalCase`
Use local labels: No
Within-subroutine label styling: `_UnderscorePrefixPascalCase`

1. There is no consensus for command name styling

Option 1: PascalCase

    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP
    PrintAttackMessage
    WaitMessage
    Wait 0x1E
    ChangeVar VAR_OP_SETMASK, VAR_SERVER_STATUS1, 0x2
    ChangeVar VAR_OP_SETMASK, VAR_MOVE_STATUS, 0x80000000
    EndScript

Option 2: alllowercase

    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP
    printattackmessage
    waitmessage
    wait 0x1E
    changevar VAR_OP_SETMASK, VAR_SERVER_STATUS1, 0x2
    changevar VAR_OP_SETMASK, VAR_MOVE_STATUS, 0x80000000
    endscript

Option 3: snake_case

    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP
    print_attack_message
    wait_message
    wait 0x1E
    change_var VAR_OP_SETMASK, VAR_SERVER_STATUS1, 0x2
    change_var VAR_OP_SETMASK, VAR_MOVE_STATUS, 0x80000000
    endscript

Option 4: camelCase

    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP
    printAttackMessage
    waitMessage
    wait 0x1E
    changeVar VAR_OP_SETMASK, VAR_SERVER_STATUS1, 0x2
    changeVar VAR_OP_SETMASK, VAR_MOVE_STATUS, 0x80000000
    endScript

2. There is no consensus for styling for "entry-point" or "subroutine" script labels

Option 1: PascalCase

CommonDamageCalc:
    CritCalc
    DamageCalc
    EndScript
CommonDamageCalc:
    critcalc
    damagecalc
    endscript
CommonDamageCalc:
    crit_calc
    damage_calc
    end_script
CommonDamageCalc:
    critCalc
    damageCalc
    endScript

Option 2: snake_case

common_damage_calc:
    CritCalc
    DamageCalc
    EndScript
common_damage_calc:
    critcalc
    damagecalc
    endscript
common_damage_calc:
    crit_calc
    damage_calc
    end_script
common_damage_calc:
    critCalc
    damageCalc
    endScript

Option 3: camelCase

commonDamageCalc:
    CritCalc
    DamageCalc
    EndScript
commonDamageCalc:
    critcalc
    damagecalc
    endscript
commonDamageCalc:
    crit_calc
    damage_calc
    end_script
commonDamageCalc:
    critCalc
    damageCalc
    endScript

Option 4: _UnderscorePrefixPascalCase Note: Technically shouldn't be done because _ prefix means a symbol is reserved. This should only matter for Trainer AI scripts which are linked with C files.

_CommonDamageCalc:
    CritCalc
    DamageCalc
    EndScript
_CommonDamageCalc:
    critcalc
    damagecalc
    endscript
_CommonDamageCalc:
    crit_calc
    damage_calc
    end_script
_CommonDamageCalc:
    critCalc
    damageCalc
    endScript

Suggest more options in the comments.

3. There is no consensus for "within-subroutine" labels

This issue is a bit more complicated because it depends on whether you use mwasmarm or not. mwasmarm has true local label support, where a label goes out of scope after a non-local label, for example:

DealDamage:
  if DAMAGE, 0, @done
  dealdamage
@done: // . at the start means it's a local label
  end

AccuracyCheck:
  if ACCURACY, 100, @done
  accuracycheck
@done: // this will not throw an error, because AccuracyCheck was defined
       // undefining the previous .done
  end

mwasmarm pros:

mwasmarm cons:

GNU assembler pros are the reverse of mwasmarm pros

GNU assembler cons (not mentioned above):

3a. If using local labels (mwasmarm assumed/forced)

Option 1: @camelCase

    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, @anyBattlerHasDamp
    AbilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @attackerHasCorrosion
    EndScript
@anyBattlerHasDamp:
    CritCalc
    EndScript
@attackerHasCorrosion:
    DamageCalc
    EndScript
    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP, @anyBattlerHasDamp
    abilitycheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @attackerHasCorrosion
    endscript
@anyBattlerHasDamp:
    critcalc
    endscript
@attackerHasCorrosion:
    damagecalc
    endscript
    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP, @anyBattlerHasDamp
    ability_check 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @attackerHasCorrosion
    end_script
@anyBattlerHasDamp:
    crit_calc
    end_script
@attackerHasCorrosion:
    damage_calc
    end_script
    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, @anyBattlerHasDamp
    abilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @attackerHasCorrosion
    endEcript
@anyBattlerHasDamp:
    critCalc
    endScript
@attackerHasCorrosion:
    damagecalc
    endScript

Option 2: @snake_case

    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, @any_battler_has_damp
    AbilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @attacker_has_corrosion
    EndScript
@any_battler_has_damp:
    CritCalc
    EndScript
@attacker_has_corrosion:
    DamageCalc
    EndScript
    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP, @any_battler_has_damp
    abilitycheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @attacker_has_corrosion
    endscript
@any_battler_has_damp:
    critcalc
    endscript
@attacker_has_corrosion:
    damagecalc
    endscript
    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP, @any_battler_has_damp
    ability_check 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @attacker_has_corrosion
    end_script
@any_battler_has_damp:
    crit_calc
    end_script
@attacker_has_corrosion:
    damage_calc
    end_script
    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, @any_battler_has_damp
    abilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @attacker_has_corrosion
    endEcript
@any_battler_has_damp:
    critCalc
    endScript
@attacker_has_corrosion:
    damagecalc
    endScript

Option 3: @PascalCase

    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, @AnyBattlerHasDamp
    AbilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @AttackerHasCorrosion
    EndScript
@AnyBattlerHasDamp:
    CritCalc
    EndScript
@AttackerHasCorrosion:
    DamageCalc
    EndScript
    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP, @AnyBattlerHasDamp
    abilitycheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @AttackerHasCorrosion
    endscript
@AnyBattlerHasDamp:
    critcalc
    endscript
@AttackerHasCorrosion:
    damagecalc
    endscript
    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP, @AnyBattlerHasDamp
    ability_check 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @AttackerHasCorrosion
    end_script
@AnyBattlerHasDamp:
    crit_calc
    end_script
@AttackerHasCorrosion:
    damage_calc
    end_script
    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, @AnyBattlerHasDamp
    abilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, @AttackerHasCorrosion
    endEcript
@AnyBattlerHasDamp:
    critCalc
    endScript
@AttackerHasCorrosion:
    damagecalc
    endScript

3b. If not using local labels

Option 1: PascalCase_WithPascalCaseSubLabel (implies PascalCase for entry-point label)

SpecialMoveAbilityCheck:
    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_AnyBattlerHasDamp
    AbilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_AttackerHasCorrosion
    EndScript
SpecialMoveAbilityCheck_AnyBattlerHasDamp:
    CritCalc
    EndScript
SpecialMoveAbilityCheck_AttackerHasCorrosion:
    DamageCalc
    EndScript
SpecialMoveAbilityCheck:
    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_AnyBattlerHasDamp
    abilitycheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_AttackerHasCorrosion
    endscript
SpecialMoveAbilityCheck_AnyBattlerHasDamp:
    critcalc
    endscript
SpecialMoveAbilityCheck_AttackerHasCorrosion:
    damagecalc
    endscript
SpecialMoveAbilityCheck:
    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_AnyBattlerHasDamp
    ability_check 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_AttackerHasCorrosion
    end_script
SpecialMoveAbilityCheck_AnyBattlerHasDamp:
    crit_calc
    end_script
SpecialMoveAbilityCheck_AttackerHasCorrosion:
    damage_calc
    end_script
SpecialMoveAbilityCheck:
    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_AnyBattlerHasDamp
    abilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_AttackerHasCorrosion
    endEcript
SpecialMoveAbilityCheck_AnyBattlerHasDamp:
    critCalc
    endScript
SpecialMoveAbilityCheck_AttackerHasCorrosion:
    damagecalc
    endScript

Option 2: PascalCase_withCamelCaseSubLabel (implies PascalCase for entry-point label)

SpecialMoveAbilityCheck:
    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_anyBattlerHasDamp
    AbilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_attackerHasCorrosion
    EndScript
SpecialMoveAbilityCheck_anyBattlerHasDamp:
    CritCalc
    EndScript
SpecialMoveAbilityCheck_attackerHasCorrosion:
    DamageCalc
    EndScript
SpecialMoveAbilityCheck:
    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_anyBattlerHasDamp
    abilitycheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_attackerHasCorrosion
    endscript
SpecialMoveAbilityCheck_anyBattlerHasDamp:
    critcalc
    endscript
SpecialMoveAbilityCheck_attackerHasCorrosion:
    damagecalc
    endscript
SpecialMoveAbilityCheck:
    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_anyBattlerHasDamp
    ability_check 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_attackerHasCorrosion
    end_script
SpecialMoveAbilityCheck_anyBattlerHasDamp:
    crit_calc
    end_script
SpecialMoveAbilityCheck_attackerHasCorrosion:
    damage_calc
    end_script
SpecialMoveAbilityCheck:
    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_anyBattlerHasDamp
    abilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_attackerHasCorrosion
    endEcript
SpecialMoveAbilityCheck_anyBattlerHasDamp:
    critCalc
    endScript
SpecialMoveAbilityCheck_attackerHasCorrosion:
    damagecalc
    endScript

Option 3: PascalCase_with_snake_case_sub_label (implies PascalCase for entry-point label)

SpecialMoveAbilityCheck:
    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_any_battler_has_damp
    AbilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_attacker_has_corrosion
    EndScript
SpecialMoveAbilityCheck_any_battler_has_damp:
    CritCalc
    EndScript
SpecialMoveAbilityCheck_attacker_has_corrosion:
    DamageCalc
    EndScript
SpecialMoveAbilityCheck:
    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_any_battler_has_damp
    abilitycheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_attacker_has_corrosion
    endscript
SpecialMoveAbilityCheck_any_battler_has_damp:
    critcalc
    endscript
SpecialMoveAbilityCheck_attacker_has_corrosion:
    damagecalc
    endscript
SpecialMoveAbilityCheck:
    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_any_battler_has_damp
    ability_check 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_attacker_has_corrosion
    end_script
SpecialMoveAbilityCheck_any_battler_has_damp:
    crit_calc
    end_script
SpecialMoveAbilityCheck_attacker_has_corrosion:
    damage_calc
    end_script
SpecialMoveAbilityCheck:
    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, SpecialMoveAbilityCheck_any_battler_has_damp
    abilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, SpecialMoveAbilityCheck_attacker_has_corrosion
    endEcript
SpecialMoveAbilityCheck_any_battler_has_damp:
    critCalc
    endScript
SpecialMoveAbilityCheck_attacker_has_corrosion:
    damagecalc
    endScript

Option 4: _underscorePrefixCamelCase, just try to avoid label conflicts

SpecialMoveAbilityCheck:
    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, _anyBattlerHasDamp
    AbilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _attackerHasCorrosion
    EndScript
_anyBattlerHasDamp:
    CritCalc
    EndScript
_attackerHasCorrosion:
    DamageCalc
    EndScript
SpecialMoveAbilityCheck:
    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP, _anyBattlerHasDamp
    abilitycheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _attackerHasCorrosion
    endscript
_anyBattlerHasDamp:
    critcalc
    endscript
_attackerHasCorrosion:
    damagecalc
    endscript
SpecialMoveAbilityCheck:
    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP, _anyBattlerHasDamp
    ability_check 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _attackerHasCorrosion
    end_script
_anyBattlerHasDamp:
    crit_calc
    end_script
_attackerHasCorrosion:
    damage_calc
    end_script
SpecialMoveAbilityCheck:
    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, _anyBattlerHasDamp
    abilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _attackerHasCorrosion
    endEcript
_anyBattlerHasDamp:
    critCalc
    endScript
_attackerHasCorrosion:
    damagecalc
    endScript

Option 5: _UnderscorePrefixPascalCase, just try to avoid label conflicts

SpecialMoveAbilityCheck:
    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, _AnyBattlerHasDamp
    AbilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _AttackerHasCorrosion
    EndScript
_AnyBattlerHasDamp:
    CritCalc
    EndScript
_AttackerHasCorrosion:
    DamageCalc
    EndScript
SpecialMoveAbilityCheck:
    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP, _AnyBattlerHasDamp
    abilitycheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _AttackerHasCorrosion
    endscript
_AnyBattlerHasDamp:
    critcalc
    endscript
_AttackerHasCorrosion:
    damagecalc
    endscript
SpecialMoveAbilityCheck:
    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP, _AnyBattlerHasDamp
    ability_check 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _AttackerHasCorrosion
    end_script
_AnyBattlerHasDamp:
    crit_calc
    end_script
_AttackerHasCorrosion:
    damage_calc
    end_script
SpecialMoveAbilityCheck:
    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, _AnyBattlerHasDamp
    abilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _AttackerHasCorrosion
    endEcript
_AnyBattlerHasDamp:
    critCalc
    endScript
_AttackerHasCorrosion:
    damagecalc
    endScript

Option 6: _underscore_prefix_snake_case, just try to avoid label conflicts

SpecialMoveAbilityCheck:
    MoldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, _any_battler_has_damp
    AbilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _attacker_has_corrosion
    EndScript
_any_battler_has_damp:
    CritCalc
    EndScript
_attacker_has_corrosion:
    DamageCalc
    EndScript
SpecialMoveAbilityCheck:
    moldbreakerabilitycheck 0x0, BATTLER_ALL, ABILITY_DAMP, _any_battler_has_damp
    abilitycheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _attacker_has_corrosion
    endscript
_any_battler_has_damp:
    critcalc
    endscript
_attacker_has_corrosion:
    damagecalc
    endscript
SpecialMoveAbilityCheck:
    mold_breaker_ability_check 0x0, BATTLER_ALL, ABILITY_DAMP, _any_battler_has_damp
    ability_check 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _attacker_has_corrosion
    end_script
_any_battler_has_damp:
    crit_calc
    end_script
_attacker_has_corrosion:
    damage_calc
    end_script
SpecialMoveAbilityCheck:
    moldBreakerAbilityCheck 0x0, BATTLER_ALL, ABILITY_DAMP, _any_battler_has_damp
    abilityCheck 0x0, BATTLER_ATTACKER, ABILITY_CORROSION, _attacker_has_corrosion
    endEcript
_any_battler_has_damp:
    critCalc
    endScript
_attacker_has_corrosion:
    damagecalc
    endScript
red031000 commented 10 months ago

Command name styling: PascalCase Entry-point label styling: PascalCase Use local labels: No (as eventually we'll want to be able to use gcc) Within-subroutine label styling: _UnderscorePrefixPascalCase (would be happy with PascalCase_WithPascalCaseSubLabel as well)

lhearachel commented 10 months ago
  1. Command name styling: PascalCase (fine with snake_case)
  2. Entry-point label styling: PascalCase (fine with camelCase)
  3. Use local labels: No (gcc-incompatible means local labels are not tenable)
  4. Within-subroutine label styling: PascalCase (fine with camelCase, as in (2)).

There is no reason to mandate subroutine prefixes on labels within the subroutine, as individual sequences are separate files in Gen4; every label within a single file is implicitly static, so the prefix is wholly unnecessary. You can have Loop as a label within as many different subroutines as you want, so long as they are hosted within separate files.

AsparagusEduardo commented 10 months ago

My choices for mirroring pokeemerald's style:

Command name styling: alllowercase Entry-point label styling: PascalCase Use local labels: No Within-subroutine label styling: PascalCase_WithPascalCaseSubLabel

luckytyphlosion commented 10 months ago

A few things I should mention:

lhearachel commented 10 months ago

We should also note that, technically speaking, labels are implicitly local in DS scripts; they're individual files within an archive, not a collection of scripts stitched together and then compiled.

mid-kid commented 10 months ago

Command name styling: gods_chosen_casing (pokeemerald was a mistake) Entry-point label styling: LuckyMoment (I hate it but why is it even up for debate) Use local labels: Are you sure both assemblers are compatible enough anyway? Stick to whatever mwasmarm supports for the time being. Within-subroutine label styling: KeepItConsistent_YouFuckingMong (again, whole lotta words for a choice that was already made)

luckytyphlosion commented 10 months ago

Are you sure both assemblers are compatible enough anyway? Stick to whatever mwasmarm supports for the time being.

Only matters for trainer AI, all other scripts are assembled and then extracted as binary anyway.

mid-kid commented 10 months ago

I wasn't talking about object file compatibility, using a different assembler for different things is a mess.

luckytyphlosion commented 10 months ago

pfero — Today at 12:49 AM

Also re local labels Does mwasmarm support gas-style numbered labels? As in

1:
   thing 1f @jumps forward
   thing 1b @jumps back
1:

If so use those tbh

luckytyphlosion commented 9 months ago

image

Seems like current consensus is probably:

  1. Command name styling: PascalCase
  2. Entry-point label styling: PascalCase
  3. Use local labels: No (gcc-incompatible means local labels are not tenable)
  4. Within-subroutine label styling: PascalCase_WithPascalCaseSubLabel
lhearachel commented 9 months ago

We have no consensus on (4).

github-actions[bot] commented 4 weeks ago

This issue has had no activity for 60 days and will be marked stale. If there is no further activity, it will be closed in 30 days.