zeldaret/af
1
0
Fork 0
mirror of https://github.com/zeldaret/af.git synced 2026-10-05 08:57:31 -04:00
Code Issues Packages Projects Releases Wiki Activity

add CONTRIBUTING.md and STYLE.md (#23)

Browse Source 
* initial commit

* review feedback

* remove early return guideline

* more info about merging main

* revise checklist item about function prototypes
This commit is contained in:
emilybrooks
2023-08-02 10:34:53 -07:00
committed by GitHub
parent 92ac058f3d
commit bc0475f78d
2 changed files with 342 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CONTRIBUTING.md
+106
View File
@@ -0,0 +1,106 @@
# Contributing
Thank you for your interest in the Animal Forest decompilation. This is a brief overview on how to contribute to the project.
## Useful Links
- [MM decomp tutorial](https://github.com/zeldaret/mm/blob/master/docs/tutorial/contents.md) Detailed tutorial for learning in general how decomp works and how to decompile a small, simple file. Note that this project uses a different build system, so some information doesn't apply.
- [Introduction to OOT decomp](https://github.com/zeldaret/oot/blob/master/docs/tutorial/contents.md) - Covers slightly different topics.
Most discussions happen in the `#af-decomp` channel on our [Discord Server](https://discord.zelda64.dev) where you are welcome to ask if you need help getting started.
## Preparing a Pull Request
You should work on a branch on your fork separate from your copy of main: it is always useful to have a clean main branch around if you need to fix something.
We squash commits when merging, so your commit history does not have to be completely spotless.
### NON_EQUIVALENT
If you are working on a function but can't figure out how to tackle certain parts, or you're sure that it's not equivalent, you can wrap it in NON_EQUIVALENT.
Please make a best-effort attempt on any function you put in a PR, though. Half-finished functions aren't necessarily much use to the next decomper.
```c
// can't figure out the MULT_HI() stuff
#ifdef NON_EQUIVALENT
void func(void) {
// ...
}
#else
#pragma GLOBAL_ASM("file/func.s")
#endif
```
### NON_MATCHING
If you are unable to 100% match a function but can verify it is equivalent in behavior, you can wrap the C code in NON_MATCHING.
```c
// s3/s4 swap
#ifdef NON_MATCHING
void func(void) {
// ...
}
#else
#pragma GLOBAL_ASM("file/func.s")
#endif
```
### Format
Run the formatting script:
```bash
python3 ./tools/format.py
```
### Merge main
To make sure the PR builds correctly with the current master, you need to merge `upstream/main` before you make the PR. While your branch is private (in particular, not used for a PR), you can rebase, but when your branch is public/used for a PR always merge main instead of rebasing: it makes it much easier for people to understand what you changed since the last review.
The simplest way to do this is to use "Sync Fork" on your branch on the GitHub website. Then sync the remote branch with your local branch using `git pull`. This tends to break things, and requires some modifications to get your branch to correctly compile again.
If the Sync Fork method doesn't work because there are merge conflicts, you can use `git merge upstream/main` instead. Manually resolve the conflicts with your text editor, make sure it builds, and then commit the changes.
## Checklist
More detailed information can be found in the [Style Guide](docs/STYLE.md).
- [ ] Jenkins build is successful.
- [ ] `make` builds a matching ROM.
- [ ] `format.py` was run.
- [ ] Any new compiler warnings that were added are required for matching.
- [ ] Files with `NON_MATCHING` functions have equivalent behavior.
- [ ] Overlays should have macros to define access to parameters if the parameter uses bitwise access. The params should have an enum when it makes sense.
- [ ] Comments and variables have correct spelling.
- [ ] Function prototypes are in the appropriate header files. Prototypes for functions that aren't meant to be called outside of the file can be omitted.
- [ ] The following should be declared in an Actor header file. There should be nothing else in the Actor header file.
- [ ] Main Actor struct
- [ ] Types used in the actor struct. Specific example would be actionFunc typedefs.
- [ ] Param field macros and/or enums.
- [ ] For any other additional `enum`/`struct`/`define`/`function`/`global`, there needs to be evidence it is needed in another file.
- [ ] Function prototypes.
- [ ] New variables and functions should follow standard naming conventions.
- [ ] Constants are converted to whichever looks best in context: hexadecimal, decimal, or float
- [ ] Rotation angles should always be in hexadecimal
- [ ] Color values should always be in decimal.
- [ ] Structs, unions, enums, and typedefs are `TitleCase` (`DmRavine`)
- [ ] Variable names are `camelCase` (`actionFunc`)
- [ ] Global variables start with `g` (`gSaveContext`)
- [ ] Static global variables start with `s` (`sSphereInit`)
- [ ] Macros and enum constants are `SCREAMING_SNAKE_CASE` (`DM_RAVINE_STATE_ACTIVE`)
- [ ] Trailing commas in array and struct definitions chosen to make it look best
## Submitting the Pull Request
Push commits to your fork of the repository on GitHub, and then open a pull request. Name the PR something sensible, like
- `EnRecepgirl OK and documented` (if all the functions match and your documentation is fairly complete)
- `EnRecepgirl OK` (if all the functions match)
- `EnRecepgirl (n nonmatching)` (if you couldn't get one or more functions to match, but to the best of your knowledge they are equivalent code)
- `EnRecepgirl (n nonequivalent)` (if you couldn't get one or more functions to match, and do not believe the code in them has the same effect)
and so on, although these four tend to cover most cases. Feel free to add a comment describing anything interesting you had to do or issues in non-matchings.
Please also update the status of the file on the spreadsheet.
docs/STYLE.md
+236
View File
@@ -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.