INCGFX: in-source arguments for gbagfx

[mrgriffin]

INCGFX is like INCBIN except that it specifies the arguments to pass to gbagfx which alleviates the user-unfriendliness caused by catch-all rules.

Specifically, users frequently forget to add to spritesheet_rules.mk when adding object event graphics which leads to garbled graphics in-game (but not in Porymap which displays them correctly). Also the built .4bpp file is not invalidated when the add their rule to spritesheet_rules.mk so they have to touch the source file or similar to un-garble their graphics.

---

The built artifacts are placed in build/assets so that they can be shared between agbcc builds and modern builds.

The INCBIN ⇒ INCGFX migration was done automatically with migrate_incgfx.py. The exact command was:

git ls-files | grep 'src/.*\.[ch]$' | xargs grep -l INCBIN | xargs python3 migrate_incgfx.py

The graphics_file_rules.mk were cleaned up manually. Mostly the kept rules are non-gbagfx rules, but there are a few rules where the outputs are used as inputs to other rules and those were kept for sanity's sake.

Notes for pokeemerald maintainers:

• In the future it would be helpful if INCGFX could handle multiple inputs so that the various @cat $^ >$@ rules and their dependencies could be removed from graphics_file_rules.mk.
• It might be nice to specify -mwidth and -mheight on all object event INCGFXs in src/data/object_events/object_event_graphics.h to future-proof against adding more frames.
• Maybe INCGFX's third argument should be optional because it's somewhat-frequently "". On the other hand, maybe it's better to be explicit because that makes the translation to a recipe more obvious, and it simplifies the code.

Notes for downstream projects:

• INCBIN still exists and I have no plans to remove it. Downstream projects can continue using INCBIN and graphics_file_rules.mk/spritesheet_rules.mk if they prefer.

Notes for Expansion maintainers:

• tools/preproc/c_file.cpp:TryConvertIncgfx introduces new calls to std::printf which must be replaced with plain printf on upcoming (and master after 1.16 is released) due to preproc: (shared) COMPOUND_STRING rh-hideout/pokeemerald-expansion#9086.
• We must also check migrate_incgfx.py correctly migrates all the INCBINs introduced in Expansion, and vendor it as a migration script. We should also double-check that it doesn't get confused on a dirty repository (e.g. one with .4bpps and .gbapals).
• We probably want to accept just the first commit (e458091 at time of writing) and reject the second commit (fb9299f at time of writing), and then recreate the second commit ourselves via the migration script.
• It might also be nice to write a migration script that removes orphaned rules from the *.mks, but as mentioned above I don't have one of those and did the changes by hand. Those rules are harmless, so they don't have to be removed.
• GriffinR plans to make a patch Porymap release soon so that graphics continue being displayed. The version with support will be 6.3.1/(Discord).

[GriffinRichards]

Incredible, this is a huge change. This makes what's happening when we include graphics in the code so much more obvious, in my opinion. So many common pitfalls relate back to our hiding things in Make rules.

Re: should we default the arguments string to empty string, yes I think so (especially if INCGFX_#("path", "extension"); would result in an error). My thought is 99% of the time that someone downstream is adding an asset, either they don't need any additional arguments, or they're adding a spritesheet to a list of existing spritesheets (which either way will have examples of how to use their -mwidth and -mheight arguments). Adding the empty strings in other cases I think would just be more work / more chances for making a mistake.

[GriffinRichards]

I think either we support the equivalent of INCBIN_S with INCGFX_S or we just get rid of INCBIN_S.

[mrgriffin]

Hmm! I can't think of any use for signed graphics files, so I'm leaning towards cutting INCBIN_S. There's no uses of it in the repo atm, and the only theoretical use I can come up with is audio... which would probably be handled by the assembler.

[GriffinRichards]

Yeesh. preproc and scaninc should probably share some form of library code. Could we maybe track this with an issue on GitHub too? I know they don't get much attention, but it's a little more visibility at least.

[mrgriffin]

Yeah, it's a real pain in the butt! A library makes sense, or perhaps preproc could absorb scaninc or vice-versa. I think now we have .d files preproc could do the job of populating them at the point the source file is processed, rather than requiring a whole separate scaninc pass that finds all the dependencies up-front... 🤔

[mrgriffin]

Ah, actually preproc can't populate the .d files because of generated files. I mean, it could but that would preproc to invoke make on all the files that don't exist but are included (via #include, INCBIN, or INCGFX). So library or perhaps merging the two tools and having, common-tool preproc and common-tool scaninc.

In any case, I'll raise an issue for it because that's not something I want to look at doing right now.

[GriffinRichards]

Yeah that's a full additional task that shouldn't hold this up

[mrgriffin]

Re: should we default the arguments string to empty string, yes I think so.

I'll make it so :)

[mrgriffin]

Pushed the changes:

• INCBIN_S* removed.
• INCGFX_U*'s third argument is optional and defaults to "".
• Added optional third argument support to migration script and re-ran it.

Could I have the chance to rebase before this is merged? I'm trying to keep the commits separate so that a downstream project can skip the migration commit.