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

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

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

Icedude907
Copy link
Contributor

@Icedude907 Icedude907 commented Nov 17, 2023

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:

  • All pokeemerald build artifacts are moved into a build/assets. This makes the number of files changed quite high, but its essentially a find/replace operation.
    In the context of graphical assets that don't have a special rule (*bpp, gbapal, lz, rl) this is optional, and could be made optional for general audio. This is to help hacks
  • Graphical rules are now inside graphics_rules.mk.
  • build/emerald is now build/emerald_modern at the request of the Discord.

Performance:

  • Initial builds are slower for a mixture of reasons (lunos4026: ~45s difference)
  • Subsequent builds are way faster than the current implementation (lunos4026: 6s opposed to 40s), but are slightly slower than #1954 due to increased mkdir calls.

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
Copy link
Member

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
Copy link
Contributor Author

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.

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.

None yet

2 participants