diff options
| author | emilybrooks <emilybrooksemilybrooks@gmail.com> | 2023-08-02 10:34:53 -0700 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2023-08-02 13:34:53 -0400 |
| commit | bc0475f78d4fd5d35ca2e824f870980eb29c765b (patch) | |
| tree | 2eb2438b3bb66375392d30134d79bdb0ae151fbf /docs | |
| parent | 92ac058f3d8dd7156c57c388f59d9c36130273f8 (diff) | |
add CONTRIBUTING.md and STYLE.md (#23)
* initial commit
* review feedback
* remove early return guideline
* more info about merging main
* revise checklist item about function prototypes
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/STYLE.md | 236 |
1 files changed, 236 insertions, 0 deletions
diff --git a/docs/STYLE.md b/docs/STYLE.md new file mode 100644 index 0000000..e193e5d --- /dev/null +++ b/docs/STYLE.md @@ -0,0 +1,236 @@ +# Animal Forest Decompilation Style Guide + +In general, documented files are a good place to look to understand this project's style. + +## Original Names +A large amount of the original names for functions, data, bss, and objects are present in the linker map files found in Doubutsu no Mori +, Animal Crossing, and Doubutsu no Mori e+. Because of this, this project aims to use the original names if known. These take priority over all code style guidelines. + +Some functions were present in DnM and removed in DnM+. You can choose a name for these, but they should match the same style as the other functions in the file, and be accompanied with a comment +```c +// Original name unknown. +``` + +Notably local variables, structs, struct members, enums, and macros are not in the linker map files. Sometimes these can be deduced from function names, debug strings, or Ocarina of Time / Majora's Mask. These original names should be used as well. + +## Types + +Use the types from `ultratypes.h`, not the standard C types: i.e. `u8`,`s8`,`s16`,`u16`,`s32`,`u32`,`f32` rather than `char`, `short`, `int`, `float` and their `signed`/`unsigned` varieties. + +## Naming + +Names are "big-endian": the most significant/largest part of the system goes first, e.g. `DM_RAVINE_STATE_ACTIVE` rather than `DM_RAVINE_ACTIVE_STATE`. + +We always write our enums and structs as `typedef`s. (Usually one can't use an enum typedef as a function argument since enum typedefs are implicitly `s32`.) + +| Type | Style | Example | +| -------------------- | ----------------------- | ----------------------- | +| Local variables | camelCase | `yawToPlayer` | +| Global variables | gCamelCase | `gSaveContext` | +| Static variables | sCamelCase | `sZeroVec` | +| Struct members | camelCase | `actionFunc` | +| Struct names | PascalCase | `EnFirefly` | +| Enum types | PascalCase | `EnFireflyMainType` | +| Enum values | SCREAMING_SNAKE_CASE | `AT_ON` | +| Defines/macros | SCREAMING_SNAKE_CASE | `SCREEN_WIDTH`,`ABS(x)` | + +## Formatting + +Before submitting a pull request, your files should be formatted with clang-format. If they aren't, Jenkins will politely remind you by failing the build. + +You can run clang-format with this python script: + +```bash +python3 ./tools/format.py +``` + +If you have some arrays or struct definitions in your file, check that they have not been obnoxiously padded out: you can usually get a better format without a final comma for short things. + +Some important things that clang-format won't fix: + +- Files should use Linux line endings (LF). +- Blank line between declarations and code: + + ```c + s32 var; + + func(); + ``` + +- Combine declarations and definitions if possible: + + ```c + s32 var = 0; + + func(); + ``` + + instead of + + ```c + s32 var; + + var = 0; + func(); + ``` + +- Blank lines between switch cases if they're long (use your judgement). + +## Numbers + +### Decimal + +- Timers +- Colors and alpha +- Usually array accesses and sizes + +### Hexadecimal + +- Angles (for now; the code itself is very inconsistent with round hex, round dec, and degrees) +- Addresses +- Bitmasks (i.e. `& 0x80` etc.) +- Struct offset comments + +Numbers below `10`/`0xA` do not need the `0x` if by themselves in code. + +### Booleans + +If a function returns only `0` or `1`, and is used as a boolean (i.e. in conditionals), replace the returns by `false` and `true`. + +### Floats + +Floats usually need an `f` on the end to match, or IDO will use doubles. Our floats are always of the form `1.0f`, even when the decimal part is zero. + +## Conditionals/Loops + +- Spacing out conditional or loop blocks from surrounding code often makes them easier to read. +- Avoid assigning or mutating variables in conditionals if possible (including `++`/`--`), avoid side effects in the loop increment slot (i.e. incrementing/assigning to loop variables is fine, something like `*a = b++` is not). +- We *always* use `{}` on conditional/loop blocks, even if they're one line (clang-tidy will enforce this). +- When conditions are `&&`d or `||`d together, use brackets around each that includes an arithmetic comparison or bitwise operator (i.e. not `!var` or `func()`, but ones with `==` or `&` etc.) +- Flag checks or functions that return booleans do not need the `== 0`/`!= 0`. + +## Arrays + +- It's better to not hardcode array sizes (easier to mod) +- Use `sizeof` or `ARRAY_COUNT`/`ARRAY_COUNTU` where it makes sense, e.g. in loops that are using an array. +- clang-format sometimes does weird things to array formatting. Experiment with and without a comma after the last element and see which looks better. + +## Play2 + +In some particular instances, IDO requires the function argument `play` to be cast to a second variable of the same type to match. In these particular instances, the function argument should be renamed to `play2` and than this `play2` just assigned to a stack variable called `play`. This cast should occur before the actor `THIS` cast is made. For example in `z_en_firefly.c` +```c +void EnFirefly_Update(Actor* thisx, PlayState* play2) { + PlayState* play = play2; + EnFirefly* this = THIS; +``` + +In other places the cast is actually not explicitly needed, but a stack `pad` variable is still needed. For this there should just be a stack variable called `pad` of type `s32` before the actor `THIS` cast. For example in `z_bg_goron_oyu` + +```c +void BgGoronOyu_Init(Actor* thisx, PlayState* play) { + s32 pad; + BgGoronOyu* this = THIS; + CollisionHeader* colHeader = NULL; +``` + +## Documentation and Comments + +Documentation includes: + +- Naming functions +- Naming struct variables +- Naming data +- Naming local variables +- Describing the general purpose of the file +- Describing any unusual, interesting or strange features of how the file or parts of its content work +- Labelling and explaining bugs +- Making enums or defines for significant numbers for the file, like actor params values. +- Naming the contents of the asset file(s) the file may use (for an actor, the object(s) it uses) + +If you are not sure what something does, it is better to leave it unnamed than name it wrongly. It is fine to make a note of something you are not sure about when PRing, it means the reviewers will pay special attention to it. + +We use comments for: + +- Top of file: a short description of the system. +- For function descriptions, we use multiline comments, + + ```c + /** + * A brief description of what the function does. + * + * More details if needed. + */ + ``` + + These are *optional*: if you think the code is clear enough, you do not need to put a comment. You can use Doxygen formatting if you think it adds something, but it is also not required. +- If something in a function is strange, or unintuitive, do leave a comment explaining what's going on. We use `//` for this. +- We also use `//` for temporary comments above a function. Feel free to use `TODO:` in these if appropriate. +- A bug should be commented with an `//! @bug Bug description` above the code that causes the bug. + +## What goes where + +This section mostly applies to actors. + +### Functions + +All functions should go in the main C file in the same order as the assembly (the latter is required to match anyway). (We may make exceptions for particularly large files with a particular organisational structure, but we ask that you check on Discord first before doing this) + +### Data + +- If in doubt, leave all the data at the top of the file. Reviewers will decide for you. +- Data must go in the same order as in the assembly files, but is only constrained by other data, not functions or rodata. +- Some data has to be inline static to match. Generally it's better to not use `static` on data outside funtions until the file is matching, since `static` data is left out of the mapfile and this makes debugging harder. +- *This is even more true of bss, where we have trouble with IDO unpredictably reordering it in certain files.* +- For small arrays or simple data that is used in only one function, we usually inline it, if it fits in the ordering. +- Generally data that is only used by the draw functions is put down near them: this is one of the few consistencies in ordering of actors' functions. + +### Enums and defines + +- Actors that bitpack params should have macros made for each access or write that is made. An undocumented example from Majora's Mask, `z_en_dg.h` + + ```c + #define ENDG_GET_FC00(thisx) (((thisx)->params & 0xFC00) >> 0xA) + #define ENDG_GET_3E0(thisx) (((thisx)->params & 0x3E0) >> 5) + ``` + + and a documented example, `z_en_firefly.h` + + ```c + #define KEESE_INVISIBLE (1 << 0xF) + #define KEESE_GET_MAIN_TYPE(thisx) ((thisx)->params & 0x7FFF) + ``` + +- In a similar manner, actors that use `home.rot.(x|y|z)` like params should also macros made for accesses and writes. (See, e.g. `z_obj_bean.h`.) +- Stuff that only the actor itself will use goes in the C file unless needed in the header. +- Anything actor-specific that might be used by another file goes in the header, in particular params access macros. +- Anything that is expected to have widespread use should go in the appropriate header file. eg z64_math.h, gfx.h, macros.h + +### Objects + +Use the original names. If the object isn't in the linker map you can match the pattern other objects use: + +Vertices +- obj_romtrain_door_v + +Display Lists +- obj_romtrain_door_model + +Skeletons +- cKF_je_r_obj_romtrain_door_tbl +- cKF_bs_r_obj_romtrain_door + +Animations +- cKF_ckcb_r_obj_romtrain_door_tbl +- cKF_kn_obj_romtrain_door_tbl +- cKF_c_obj_romtrain_door_tbl +- cKF_ds_obj_romtrain_door_tbl +- cKF_ba_r_obj_romtrain_door + +Textures +- obj_romtrain_door_pal +- obj_romtrain_door_tex_txt + +## Above all else + +*All of the above is subservient to matching.* Sometimes IDO cares about newlines, for example. + +If you are not sure about any of the above, please ask in Discord. |
