diff options
| author | emilybrooks <emilybrooksemilybrooks@gmail.com> | 2023-08-28 09:55:24 -0700 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2023-08-28 09:55:24 -0700 |
| commit | fcfbb6c52feec4a01b46f0c2bcc037af373d4fe9 (patch) | |
| tree | ce8c8c267c1dfd5f3ba7b0317ab8b2c7c317b9cd | |
| parent | 94c8f704af3fe2f4849a7f3187d3977ea6c763c5 (diff) | |
update contributing.md and style.md (#76)
* initial commit
* fixed wording
* more fixed wording
* review
| -rw-r--r-- | CONTRIBUTING.md | 31 | ||||
| -rw-r--r-- | docs/STYLE.md | 42 |
2 files changed, 51 insertions, 22 deletions
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 591e759..4d461e2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,9 +9,28 @@ Thank you for your interest in the Animal Forest decompilation. This is a brief 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 +## Tips for Successful Pull Requests -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. +- Don't bundle together multiple files worth of decompiled code into a single PR. Large PRs require a lot of effort to review, and make things difficult for everyone. +- You don't necessarily have to decompile a whole file, you can just decompile a couple of functions in a single PR if that suits your workflow better. +- Review the [Style Guide](docs/STYLE.md) and see if there's anything you can preemptively fix before code reviews. +- After a PR is opened always merge main instead of rebasing. Rebasing changes how commits are shown in PRs, making changes difficult to follow. + +## Making a Pull Request + +You should work on a branch on your fork separate from your copy of `main`. You can create branches from `upstream/main` so you don't have to sync your `main` branch every time. + +If you haven't done so already, add the main repo as `upstream` + +```bash +git remote add upstream https://github.com/zeldaret/af.git +``` + +Then create new branches with this: + +```bash +git fetch upstream && git checkout -b new-branch-name upstream/main +``` We squash commits when merging, so your commit history does not have to be completely spotless. @@ -57,11 +76,9 @@ 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. +To make sure the PR builds correctly with the current main, you need to merge `upstream/main` before you make the PR. 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 fetch && 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. +If the Sync Fork method doesn't work because there are merge conflicts, you can use `git fetch upstream && git merge upstream/main` instead. Manually resolve the conflicts with your text editor, make sure it builds, and then commit the changes. ## Checklist @@ -103,4 +120,4 @@ Push commits to your fork of the repository on GitHub, and then open a pull requ 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. +Please also update the status of the file on the [spreadsheet](https://docs.google.com/spreadsheets/d/1IkamT3ZFhgtZAaS5HS47b2_PcLlYmEFWk2oUPDhYfI8/). diff --git a/docs/STYLE.md b/docs/STYLE.md index a3bd6c6..15c3d58 100644 --- a/docs/STYLE.md +++ b/docs/STYLE.md @@ -5,6 +5,8 @@ In general, documented files are a good place to look to understand this project ## 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. +These .map files can be found pinned in the `#af-decomp` channel in our discord. + 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. @@ -12,6 +14,8 @@ Some functions were present in DnM and removed in DnM+. You can choose a name fo 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. +If you are simply decompiling a file and are not entirely sure what the function's name should be, or what a struct should be called, It is preferable to leave functions unnamed, or use unk_struct_XXX for structs, unk_XXX for struct members, etc. If something is named incorrectly it can be confusing to correct in the future. + ## 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. @@ -114,22 +118,23 @@ Floats usually need an `f` on the end to match, or IDO will use doubles. Our flo - 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 +## game_play2 + +In some particular instances, IDO requires the function argument `game_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 `game_play2` and than this `game_play2` just assigned to a stack variable called `game_play`. This cast should occur before the actor `THIS` cast is made. For example: -In some particular instances, IDO requires the function argument `game_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 `game_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, Game_Play* play2) { - Game_Play* game_play = play2; - EnFirefly* this = THIS; +void aTOU_actor_init(Actor* thisx, Game_Play* game_play2) { + Game_Play* game_play = game_play2; + Structure* 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` +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: ```c -void BgGoronOyu_Init(Actor* thisx, Game_Play* game_play) { +void aTOU_actor_ct(Actor* thisx, Game_Play* game_play) { s32 pad; - BgGoronOyu* this = THIS; - CollisionHeader* colHeader = NULL; + Structure* this = THIS; + s32 type = (common_data.time.season == WINTER); ``` ## Documentation and Comments @@ -165,20 +170,27 @@ We use comments for: - 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. +- For "fake matches", awful code that exists only to make the function's assembly match, mark these with `//! FAKE` ## 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) +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 organizational structure, but we ask that you check on Discord first before doing this) + +### Prototypes + +Actors: +- Only make prototypes that are necessary. Because actors are usually self contained, prototypes should be placed in the main C file, unless they are used by something outside of the file, where they should be placed in the header. + +Other files (boot, code, etc): +- Prototypes should be placed in the header. You can omit functions that shouldn't be used outside the file. ### 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. +- Do not use `static` on data outside functions, since `static` data is left out of the mapfile and this makes debugging harder. Only use static when necessary to match. - *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. @@ -192,14 +204,14 @@ All functions should go in the main C file in the same order as the assembly (th #define ENDG_GET_3E0(thisx) (((thisx)->params & 0x3E0) >> 5) ``` - and a documented example, `z_en_firefly.h` + and a documented example from Majora's Mask, `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`.) +- 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` from Majora's Mask.) - 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 |
