ZigGBA icon indicating copy to clipboard operation
ZigGBA copied to clipboard

Add image to tiles build tool, add sound API, and add a music example utilizing both

Open pineapplemachine opened this issue 5 months ago • 4 comments

Video of the jesuMusic example ROM: https://www.youtube.com/watch?v=3aYsL-UQp8M

The music data in jesuMusic.zig was originally composed in Dn-FamiTracker, before being transcribed to the format used in the Zig code there - jesu.zip

This is a bigger PR with three big changes lumped together:

  1. Added a build-time tool for converting images to tiles, renamed the assertconverter directory to simply build, put the tiles tool here, and moved builder.zig into it too. (The intention is that this directory can contain all the build-time tools.)
  2. Added an API for dealing with sound-related registers.
  3. Added an example that uses the first and also the PSG channels of the latter.

With a few minor changes too, such as:

  1. Minor additions to display.zig and bg.zig, primarily in the form of doc comments.
  2. Minor fixes to oversights in the secondsTimer example as I learn a bit more about Zig and the codebase.
  3. Added a build.zig.zon. (Makes the project work nicely with anyzig.)

It would probably be valuable to anyone else considering making homebrew with this to have an example maxmod integration, but for my purposes right now I only need PSG, so that was what I focused on in the example.

I am definitely open to iterating on the API used here. And I do actually have suggestions to make for possible changes that are not currently included in the PR (maybe for consideration here, maybe for a subsequent PR?):

  • Unless it's a very standard Zig thing that more experienced Zig devs than me would expect, I would prefer to name enum members things like size_32x32 over @"32x32". The first is more easily readable as being an identifier, and also a little faster to type.
  • I suggest adopting the same bpp_4 and bpp_8 enum member names as currently used in the new tiles tool in color.zig, instead of color_16 and color_256. In most places where these enums are used, what really matters is describing bits per pixel for the sake of memory layout, rather than number of colors per pixel, making the former clearer and easier to understand in my opinion.
  • I suggest that examples should use explicit gba.etc symbols instead of defining aliases at the top of the module, since this would make them easier to read and understand as documenting examples, leaving no mystery to what declarations came from the gba module and which came from somewhere else.
  • I found it confusing and inconvenient to have tiles and bg layer and obj code spread across multiple modules, especially considering how all of these things operate on potentially overlapping sections of VRAM. I think it would be an improvement to consolidate all this into one "display" module.
  • I'd really like to rename DivResult.divisor to DivResult.quotient, this trips me up every time I look at it.

Obvious opportunities for future improvments:

  • Include an example that makes use of the DSound channels, e.g. by using maxmod.
  • Include an example that makes use of the waveform PSG channel.
  • Include an API to have the new tile conversion tool use indexed color as-is. (Currently, indexed color is converted to truecolor and then converted to a palette color. This is desirable for my own use case but won't be for everyone's.)
  • The new helper functions for memcpy'ing into VRAM should panic/report an error if the source isn't word-aligned, if that's feasible. (And maybe it can use DMA, or there should be DMA variants?)
  • Use the new tile helper for other examples too.

pineapplemachine avatar Jun 04 '25 18:06 pineapplemachine

This looks like a great start! I have a lot of API suggestions, but before getting bogged down in a review I'll just respond to a couple of things:

* Unless it's a very standard Zig thing that more experienced Zig devs than me would expect, I would prefer to name enum members things like `size_32x32` over `@"32x32"`. The first is more easily readable as being an identifier, and also a little faster to type.

I went back and forth on this SO much during my initial refactor. I ended up going with this because it's what the actual language does for keyword named enum variants in the standard library like @"struct". That said, I still hate it and am not opposed to making a change. I also dislike the old C tradition of prefixing enum variants with something like the enum (which like your examples is one of the more natural alternatives in cases like this). I almost went with w32h32 for Size, and that might still be my first choice of alternative? Although, since Zig enums are mostly used as literals, it's probably better than most languages to prefix them with the name.

* I suggest adopting the same `bpp_4` and `bpp_8` enum member names as currently used in the new tiles tool in `color.zig`, instead of `color_16` and `color_256`. In most places where these enums are used, what really matters is describing bits per pixel for the sake of memory layout, rather than number of colors per pixel, making the former clearer and easier to understand in my opinion.

Hard agree on making bits per pixel the semantic meaning for that enum, I think that's much better.

* I suggest that examples should use explicit `gba.etc` symbols instead of defining aliases at the top of the module, since this would make them easier to read and understand as documenting examples, leaving no mystery to what declarations came from the gba module and which came from somewhere else.

I understand the thought, but liberal use of type aliases/re-exports is a pretty core part of most zig code that I've seen, and one of the goals of the examples is to try to give an example of plausible real-world usage.

* I found it confusing and inconvenient to have tiles and bg layer and obj code spread across multiple modules, especially considering how all of these things operate on potentially overlapping sections of VRAM. I think it would be an improvement to consolidate all this into one "display" module.

I thought about this, and I think I'd be okay putting bg inside display but OAM is a separate portion of memory than VRAM. I do think there's room for improvement, and I'm not really afraid to make breaking changes.

* I'd really like to rename `DivResult.divisor` to `DivResult.quotient`, this trips me up every time I look at it.

I can't believe I haven't already done this, I think I even call the temporary quo in the bios asm function.

vortexofdoom avatar Jun 04 '25 20:06 vortexofdoom

Hard agree on making bits per pixel the semantic meaning for that enum, I think that's much better.

Since we're hard agreed on this one and since this PR touches related code already I'll go ahead and include this change in this PR, so the two color depth enums can be consistent. As for the others, maybe for a subsequent refactor PR, since this one is already pretty bulky as it is?

I understand the thought, but liberal use of type aliases/re-exports is a pretty core part of most zig code that I've seen

In most cases I would bow to convention, but in this case I'll argue that this is a very poor convention. Explicit namespacing is a big help for code readability in general, and I think quite important in code that is meant to serve as a documenting example. The extra indirection for symbol resolution has to be noticed and kept track of, and results in reading and following the examples taking more effort than otherwise.

pineapplemachine avatar Jun 04 '25 20:06 pineapplemachine

Something that has been increasingly nagging at me:

GBATEK says bits 5 and 6 of SOUND3CNT_L can be used to switch between two banks of waveform sample data, or to treat both banks as one double-long sample. A single bank contains 32 samples, each sample is 4 bits, meaning each bank is 16 bytes.

https://problemkaputt.de/gbatek.htm#gbasoundchannel3waveoutput

image

But just below this, GBATEK only documents registers for a single 32-sample/16-byte bank.

image

It also documents this block of registers 0x04000090 to 0x0400009F being followed immediately by FIFO_A_L at 0x040000A0, meaning there's no room for another 16 byte bank to immediately follow the last register of the documented bank.

https://problemkaputt.de/gbatek.htm#gbasoundchannelaandbdmasound

image

Nor is there room in the immediately lower addresses, with addresses 0x04000080 to 0x0400008F being used for other sound control registers.

image

https://problemkaputt.de/gbatek.htm#gbasoundcontrolregisters

Here the equivalent GB/GBC register NR30 is documented as having the corresponding bits unused, which doesn't strictly assert anything about the GBA register but it certainly isn't any help.

https://gbdev.io/pandocs/Audio_Registers.html#ff1a--nr30-channel-3-dac-enable

image

And then there's this page, which nonsensically claims that the memory address range 0x04000090 to 0x0400009F contains 64 samples/32 bytes. (It doesn't. It contains 32 samples/16 bytes.)

https://gbadev.net/gbadoc/audio/sound3.html#sound-channel-3-demo

image

Where is this alleged second waveform sample bank???

Edit: I have been informed on the gbadev Discord that both banks share the same range of registers, and the bank you affect by writing to these registers is whichever bank is not currently set via bit 6. I'll add notes about this in comments at some point. I also now realize that the last linked page does kind of explain this, even if I think not very clearly...

pineapplemachine avatar Jun 05 '25 20:06 pineapplemachine

In most cases I would bow to convention, but in this case I'll argue that this is a very poor convention. Explicit namespacing is a big help for code readability in general, and I think quite important in code that is meant to serve as a documenting example. The extra indirection for symbol resolution has to be noticed and kept track of, and results in reading and following the examples taking more effort than otherwise.

I find it dramatically more difficult to read code that has a lot of explicit namespacing, especially the same paths used over and over. It adds a ton of visual clutter for things that I only need to read once or twice to remember. I find explicit namespacing helpful if I'm reading a line or two of code with no extra context, but as I read more and familiarize myself it quickly becomes much more a hindrance than a help.

That said, I'm aware that's personal preference, and I certainly won't mandate that examples avoid extensive explicit namespacing, I'm just not inclined to add it, either.

vortexofdoom avatar Jun 06 '25 20:06 vortexofdoom