Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decompile pl_batt_obj NARC, document related battle_icon.c #269

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

joshua-smith-12
Copy link
Contributor

@joshua-smith-12 joshua-smith-12 commented Oct 15, 2024

  • Decompile pl_batt_obj.narc
  • Implement Python scripts to build the NARC
  • Rename unk_0207C908 -> battle_icon and document (some) fields

TODO:

  • update any unfixed PNG files from this NARC with the correct width + palette

this is my first contribution, tried my best to keep it to a similar style as the rest of the code but I probably scuffed it a little :)

Copy link
Collaborator

@lhearachel lhearachel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some preliminary comments on the assets before I dig into code. Overall, I'd like to re-evaluate the structure and have a good strategy for both how we build these files back into a single NARC and also how we present them to users.

res/battle/graphic/pl_batt_obj/palettes/meson.build Outdated Show resolved Hide resolved
res/battle/graphic/pl_batt_obj/sprites/data.json Outdated Show resolved Hide resolved
src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Outdated Show resolved Hide resolved
__attribute__((aligned(4))) static const u16 ov16_BattlePlatformIndexToNARCMember[] = {
* Maps a battle terrain index to the NARC member in pl_batt_obj containing a sprite for it.
*/
__attribute__((aligned(4))) static const u16 ov16_BattleTerrainIndexToNARCMember[] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

polish: Same suggestion about types/classes applies here; there is a set of terrain constants defined in consts/terrain.json, which gets compiled to build/consts/terrain.h.

...and looking at the names there, it will likely be better for us to align the names there with the names from these terrain blobs.

Copy link
Collaborator

@lhearachel lhearachel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions for cleaning up names

src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Show resolved Hide resolved
src/battle/ov16_02268520.c Outdated Show resolved Hide resolved
src/battle/ov16_02268520.c Outdated Show resolved Hide resolved
src/battle/ov16_02268520.c Outdated Show resolved Hide resolved
@@ -134,26 +135,26 @@ static BOOL ov12_02237474(BallRotation *param0);
* These NARC members describe the sprite, palette, cell, and anim to use for that ball.
*/
static const int ov12_BallIndexToNARCMembers[][4] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static const int ov12_BallIndexToNARCMembers[][4] = {
static const int sThrownBallSpriteResources[][4] = {

Also check if it's possible to turn this into an array of structs like these (this name is likely not unique):

struct SpriteResource {
    int tiles;
    int palette;
    int cells;
    int animation;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might need some advice on this one, the array gets used later as sThrownBallSpriteResources[v0][param1], so if I convert it to an array of struct the [param1] would be trying to index that struct as if it's an array

I can do something like the below and then have the array access use the array part of the union but idk if this is good style. Or is there a better alternative to make this work?

typedef union AnimatedSpriteResource {
    struct AnimatedSpriteResource_s {
        int tiles;
        int palette;
        int cells;
        int animation;
    };
    int AnimatedSpriteResource_a[4];
} AnimatedSpriteResource;

@joshua-smith-12
Copy link
Contributor Author

joshua-smith-12 commented Oct 17, 2024

Started looking at NCER/NANR, will require a couple nitrogfx changes which I will note here

red031000/nitrogfx#14
red031000/nitrogfx#15

Comment on lines 248 to 250
pl_batt_obj_00000247.ncgr
pl_batt_obj_00000248.ncer
pl_batt_obj_00000249.nanr
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these 3 files (along with pl_batt_obj_00000079.nclr) appear to be a duplicate of the poke_ball_throw components; have not found where they are used in the code to validate

Comment on lines 268 to 283
pl_batt_obj_00000267.ncer
pl_batt_obj_00000268.ncer
pl_batt_obj_00000269.ncer
pl_batt_obj_00000270.ncer
pl_batt_obj_00000271.ncer
pl_batt_obj_00000272.ncer
pl_batt_obj_00000273.ncer
pl_batt_obj_00000274.ncer
pl_batt_obj_00000275.nanr
pl_batt_obj_00000276.nanr
pl_batt_obj_00000277.nanr
pl_batt_obj_00000278.nanr
pl_batt_obj_00000279.nanr
pl_batt_obj_00000280.nanr
pl_batt_obj_00000281.nanr
pl_batt_obj_00000282.nanr
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are almost certainly corresponding with the player_..._back sprites, but have not found where they are used in the code to confirm

@joshua-smith-12 joshua-smith-12 marked this pull request as ready for review October 28, 2024 18:07
@@ -0,0 +1 @@
subdir('pl_batt_obj')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Can we name the internal folder something more meaningful? Maybe res/battle/graphics/objects? We generally try to avoid using the actual NARC names outside of where strictly necessary for matching (e.g., on NARC names)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed the directory, I considered renaming some of the other files (pl_batt_obj.ignore, pl_batt_obj.order) but these are more directly related to the final NARC name so I've left them as-is

@@ -0,0 +1,228 @@
healthbox_doubles_opponent_2.NCER
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Maybe we want to discuss with red about adding lz encoding as a switch on nitrogfx outputs so that we can avoid having huge ignore files like these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this would probably be helpful. i suspect there will be other large NARCs in the project with lots of compressed files

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll write up a PR to implement this over the course of the next couple of days.

src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Outdated Show resolved Hide resolved
src/battle/battle_icon.c Show resolved Hide resolved
Copy link
Collaborator

@lhearachel lhearachel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks generally mergeable to me at this juncture. Resolve the conflict in tools/scripts/meson.build and it should be good. I'll hold off on approval until we have the new nitrogfx option to automatically LZ-encode an output file so that we can clean up the ignore file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants