pret / pokeemerald

Decompilation of Pokémon Emerald
2.2k stars 2.38k forks source link

[Build System Rewrite] Everything at once: Includes `build/assets` #1958

Closed Icedude907 closed 2 weeks ago

Icedude907 commented 10 months ago

This pull request incorporates all changes I have made in relation to the build system. I consider this a draft while the others haven't been merged, but you could merge this and it would include all of the following:

To those of you considering my PRs, the intention was to subdivide changes to make them easier to adopt, as not all changes have the same levels of impact. Please read through them so you know which does what before discussion.

Additionally:

Performance:

Additional comments

I don't particularly want to be working much more on this. Its taken much more effort than I originally planned.

Discord contact info

icedude907

GriffinRichards commented 10 months ago

Just commented on the non-drafts for now. For this one though I will say I'd still much rather have build artifacts outside build than introduce the build/assets path changes throughout the repo, primarily because I'm anticipating two points of confusion from novice users:

  1. Some INCBIN paths will be prefixed with build/assets and some won't (namely uncompressed .bin files). I suspect at some point someone will do INCBIN(build/assets/graphics/my_tilemap.bin);. If that works as expected then it's not a big deal, but I'm sure we'll get questions about why there's a difference in paths.
  2. I'm not sure this alleviates the "how do I make a 4bpp file" problem people get confused about, which to me is a major appeal of hiding these artifacts away in build. Because all the INCBIN paths specify the build folder I imagine people will still follow it to find a folder full of 4bpp files etc., and wonder how to edit them / make their own. If people instead follow it to the root graphics folder they'll find a .png of the same name, which is a little clearer (Aside, even more ideally we could specify a path to a .png, and gbagfx could interpret the bit depth as needed or from a specified option. That way .4bpp files are completely out-of-sight).

With those in mind I'm wondering whether moving the generated files to build is worth the widespread changes and conflicts. The upsides I can think of are that you don't need to scroll past them in the graphics folders (and helps point 2 above for people browsing graphics) and it avoids some potential pitfalls of editing generated files (which atm is "handled" by warning labels at the top, though in both cases people will still continue to edit these by mistake when errors direct them to those files). I don't think I'd consider those upsides worth the trouble (obviously this is all just my opinion).

Icedude907 commented 9 months ago

These are some very good points.

For generated code, I think build/assets is worth due to reduced pitfalls as you mentioned. Additionally, all includes of those files now have generated/ on their path, which should make their nature more obvious.

In terms of improving clarity around INCBIN... For me the biggest thing was reducing clutter in the filesystem so I hadn't actually thought much on introducing that perceived inconsistency between some types of files. There's already a lot of wizardry going on (e.g.: conjuring .gbapal from .png and .aif files seemingly becoming .bin files) so maybe we should insert some basic documentation in INSTALL.md which can be read while users are doing the initial build. In terms of a more comprehensive solution, some kind of macro like INCBIN_GENERATED("path/to/source.png", 4bpp) could be an option. Scaninc + the preprocessor could sort the rest.

As an aside about .bin tilemaps, it might be worth giving them a separate extension (like .tilemap). That'd make their purpose more obvious and would make a file explorer's "open in default app" a useful option.

GriffinRichards commented 2 weeks ago

I'm going to close this for now while I'm merging a bunch of these changes from their individual PRs, but I'd love to reopen it if there's renewed interest. I think the suggestions about improving INCBIN clarity are good, I'm otherwise anticipating those points of confusion I mentioned.

Icedude907 commented 1 week ago

Thanks @GriffinRichards. I appreciate your response.