Skip to content

src: declare all charmaps explicitely #449

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

Merged
merged 2 commits into from
Oct 2, 2022
Merged

src: declare all charmaps explicitely #449

merged 2 commits into from
Oct 2, 2022

Conversation

kemenaran
Copy link
Collaborator

This PR explicitly declare all charmaps used throughout the code - without defaulting on ASCII as it previously did. This fixes -Wunmapped-char warnings on the (yet unreleased) rgbds 0.6. Credits to @ShadowOne333

Implementation

  • The shared constants now define both an AsciiCharmap and a JpCharmap.
  • The AsciiCharmap has variations per revision.
  • A CreditsCharmap is derived from the AsciiCharmap, with additional patches per-language.

This is slighly different from the implementation proposed by @ShadowOne333, but should work the same.

@ShadowOne333
Copy link

The proposed changes look really good, and a much better implementation than my hacky method for sure :P

@kemenaran kemenaran merged commit ecdaf99 into main Oct 2, 2022
@kemenaran kemenaran deleted the explicit-charmaps branch October 2, 2022 11:08
@ShadowOne333
Copy link

ShadowOne333 commented Oct 3, 2022

Tested this change, but I am still getting warnings with the credits code. For example:

warning: src/main.asm(279) -> src/code/credits.asm(137) -> src/text/credits.asm(63): [-Wunmapped-char]
    Unmapped character ' '

This still appears on all revisions of LADX.
The credits warning might be a simple solution of adding the proper charmap or PUSHC into the correct places.
Adding include "constants/charmaps/main.asm" at the begining of each of the credits.asm files inside /src/text/ (or the corresponding files for the revision ones) seems to fix the warning.

So it's most likely that there's an issue with the code inside /src/code/credits.asm, or most likely the text/credits.asm file is not being read properly, since changing the SETCHARMAP CreditsCharmap to SETCHARMAP AsciiCharmap also fixes the issue:

PUSHC
SETCHARMAP CreditsCharmap
include "text/credits.asm"
POPC

EDIT: Seems like after updating to a recent commit of rbgds, I'm no longer getting the charmap warnings, sorry about that.
Although, one thing to consider is that the checksums now no longer match those of the originals.
The problems seems to be related to the incbin "gfx/items/slime_key.dmg.2bpp" file, which is line 123 inside main.asm. Changing the slime_key.dmg.2bpp to slime_key.cgb.2bpp fixes the mismatching md5 checksum.

Comparing the original azle-r2.gbc against the new one, the only differences seem to be within this file. For some reason, this specific graphic isn't matching anymore. Not sure if the cgb is the intended one by default or if the dmg one is the right one.


Additionally, there's also these two -Wobsolete warnings still remaining:

warning: src/main.asm(16) -> src/code/bank0.asm(5) -> src/code/home/init.asm(12): [-Wobsolete]
    ld optimization will stop being the default; pass `-l` to opt into it
warning: src/main.asm(16) -> src/code/bank0.asm(6) -> src/code/home/render_loop.asm(368): [-Wobsolete]
    `nop` after `halt` will stop being the default; pass `-H` to opt into it

The former seems to be related to automatic optimization, rgbds changes ld automatically to ldh, but starting on 0.7.0 they will drop this. We can use -l to opt in again, or rewrite all the ld [rXXXX], a and ld a, [rXXXX] commands to use ldh instead of ld.

The latter seems to be related to the fact that rgbds adds an nop after a halt by default. Using -H reenables this, though I'm not sure currently how to fix it at the moment without forcing the -H flag.

@tobiasvl
Copy link
Collaborator

tobiasvl commented Oct 14, 2022

Additionally, there's also these two -Wobsolete warnings still remaining:

Is fixed in #451

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

Successfully merging this pull request may close these issues.

5 participants