-
Notifications
You must be signed in to change notification settings - Fork 21
Change the bit number/flag naming conventions #58
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
Conversation
A large change like this deserves scrutiny -- make sure I didn't get any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't have any nitpicks with the names. Thank you!
Aside: Here's a little utility function for getting the formatting right: def align_on(token, paragraph):
token = f' {token} '
lines = [line.split(token, 1) for line in paragraph.split('\n')]
lines = [[p[0].rstrip(), p[1].lstrip()] if len(p) == 2 else [p[0].rstrip()] for p in lines]
size = max(len(p[0]) for p in lines if len(p) == 2)
lines = [p[0].ljust(size) + token + p[1] if len(p) == 2 else p[0] for p in lines]
return '\n'.join(lines) |
4dc7bf9
to
718b901
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'm a big fan of these changes.
def JOYPF_LEFT equ 1 << JOYPB_LEFT | ||
def JOYPF_RIGHT equ 1 << JOYPB_RIGHT | ||
def B_JOYP_GET_BUTTONS equ 5 ; 0 = reading buttons [r/w] | ||
def B_JOYP_GET_CTRL_PAD equ 4 ; 0 = reading Control Pad [r/w] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know it was called "control pad", I'm quite used to "dpad". This is the one change I would hesitate on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's officially "Control Pad" (and likewise "Game Boy" not "Gameboy", "Game Link Cable" not "link cable", etc). I could shorten this to D_PAD
, if it's widely preferred by users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually kinda preferred Dpad as well. But just a preference.
One thing I'm tempted to do is move Searching " |
This is the first major-version update to hardware.inc since 4.0 in 2021, and is updating practically all the non-register names, so now's the time to think about whether any other names could be improved. Suggestions are welcome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really really like this change! I'd be fine with converting pret to use this now. I hope other people in the gbdev community weighs in.
I've said everything I have to say about breaking changes, and I think this is a losing battle so I think I will mostly shut up about it going forward. But I will say this. Instead of breaking |
@nitro2k01 Thank you for leaving negative feedback here too. It's worth addressing: why change hardware.inc instead of making something else? The short answer is, GBDev recommends hardware.inc to new users -- in Pan Docs, in gb-asm-tutorial, in its own repository list -- so the name "hardware.inc" needs to be on the latest and most maintained version. For established users like yourself who are already used to names like hardware.inc hasn't been afraid of breaking changes before. 3.0 did a few renames (e.g. Note that when changes go further than this, @ISSOtm and I have made a separate repo, https://github.com/Rangi42/hardware2.inc. That changes even the official register names, e.g. Footnotes
|
Note that issue #26 was never definitively rejected by any maintainers, but just left to sit open for over 3 years. That shouldn't happen. Either the project maintainers can say "the names are what they are, no more major versions, deal with it", or we can be open to proposing, hashing out, and then finally accepting new changes. But leaving the idea open and neglected implies that there's just no strong will to do anything in particular -- keeping things as they are, not because they're really preferred, but just out of inertia. Nobody wanting to volunteer and figure out what the bold new changes should be. (Frankly nobody was maintaining hardware.inc -- even the users who were comfortable with the traditional constants never bothered completing the set of bit+flag names to match Pan Docs. And I know that @aaaaaa123456789, at least, just copy+pastes his needed subset of names, and doesn't even bother with bit/flag constants, just raw numbers. So I suspect that what was happening was, the old experienced users didn't need hardware.inc to be a complete resource anyway, and the new users were just not being well-served.) (Also, out of the two maintainers for gbdev's asm-related projects, I'm the more conservative one; be glad that |
One more thing -- the motivation for actually making this v5 PR was that I tried to use the v4 names in pokecrystal (pret/pokecrystal#1186), replacing pret's accreted subset of names, and realized "...this is a step backwards". Who wants to replace pret's hardware_constants.asm had grown some names for registers and bits that Pan Docs described but hardware.inc itself didn't, so I ported those to it in 4.x. And then decided, let's not keep neglecting #26, if we're not closing it then let's address it. |
def LCDC_ON equ 1 << B_LCDC_ENABLE | ||
def LCDC_WIN_MAP equ 1 << B_LCDC_WIN_MAP | ||
def LCDC_WIN_9800 equ 0 << B_LCDC_WIN_MAP | ||
def LCDC_WIN_9C00 equ 1 << B_LCDC_WIN_MAP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like being able to explicitly write 9800 or 9C00, instead of just implying 9800 via the absence of 9C00
The tutorials could just as well point to the new thing, assuming it was branched off to a whole new repo.
Ok, but those were likely almost never used in practice. I'm more likely to complain about changes that break 100% of projects without exception. (Unless you count a project that somehow doesn't use a single constant anywhere.)
Yes, it is a community convention, or more like convention of Also note with the example of And yes, this is why (even just on principle) I prefer keeping a copy of Time for a round of rate your constant:
Ok, I admit,
So here's an issue with
There aren't even any bit mask constants, aside from And for the OAM flags:
If it has the My general opinion on unabbreviating the OAM and LCDC constants is: don't if you can help it. You will often have a long list of them |
(I see you have suggestions on the individual constant names, great! Will address that separately.)
Everyone whose project copy+pastes hardware.inc, or includes it as a Git submodule, will be using a static unbreakable copy. Anyone who tries updating it from v4 to v5 will realize the breaking change, hopefully because they were using the I guess we could create a gbdev/hardware2.inc repo, use hardware2.inc in resources like Pan Docs and gb-asm tutorial, announce that hardware.inc is an historical artifact which won't be getting any more major version updates, and archive this repo. But what's the advantage of that? It reifies "hardware.inc" from "the name for the thing we use for hardware constants, which continuously evolves" to "this particular file Jeff Frohwein made, which is of historical interest but now GBDev suggests using something else". The RGBDS assembler has changed more drastically than this, step by step, but at no point did we say "this isn't
I agree it would be unfriendly and unhelpful to break repos without any solution. hardware_compat.inc is a drop-in solution for that. If someone's putting in the effort to edit + build + commit project updates anyway, and updates their hardware.inc copy or submodule, then this one extra line isn't a big deal. |
I agree with @nitro2k01 regarding the name change. First, you're creating a new and incompatible header, so you might as well give it a new name. But also, |
I was hoping that the existence of
I totally agree! Although for its own game-engine constants pokecrystal tends to define
Yeah lol, and
True, and that would be one downside to pokecrystal switching to hardware.inc: I'm used to I could be persuaded to abbreviate (Likewise Given that this update no longer has an ; old: 133 chars
ld a, LCDCF_OFF | LCDCF_WIN9800 | LCDCF_WINOFF | LCDCF_BLK21 | LCDCF_BG9800 | LCDCF_OBJ16 | LCDCF_OBJOFF | LCDCF_BGOFF | LCDCF_PRIOFF
; new: 141 chars
ld a, LCDC_OFF | LCDC_WIN_9800 | LCDC_WINDOW_OFF | LCDC_BLOCK21 | LCDC_BG_9800 | LCDC_OBJ_16 | LCDC_OBJ_OFF | LCDC_BG_OFF | LCDC_PRIORITY_OFF
; abbreviated WIN and PRI: 133 chars
ld a, LCDC_OFF | LCDC_WIN_9800 | LCDC_WIN_OFF | LCDC_BLOCK21 | LCDC_BG_9800 | LCDC_OBJ_16 | LCDC_OBJ_OFF | LCDC_BG_OFF | LCDC_PRI_OFF |
Didn't we have this same sort of question in gbdev/rgbds#1609? Lots of people know and refer to "hardware.inc" and "rednex gb dev system", so <the repos that are our "official" hardware include file or assembler for the community> should keep those old names, even as they change to become incompatible. (Why have semver if we don't do any major updates?) |
The discussions are quite different. In the discussion of your link, the meaning of the R would only affect a couple of mentions in the documentation. This discussion is about a general push to change the register names used for decades to new names. A push that affects... well, everything. Everything has used the old names for ages: documentation, Most libraries don't do big breaking changes because the maintainers don't want to annoy their users. Maintaining something doesn't mean you have to change things that work. It can also mean not doing anything but fixing bugs. It can also mean making improvements in a backwards-compatible way. A major version increment, according to semver, can mean any breaking API change from this:
To this:
I've stopped using libraries over breaking changes that were too big. This is also why I also think that a completely new repository would make more sense. You can point to it from the documentation and leave Are you free to make breaking changes to The context in the NDS homebrew scene when the big rename happened was very different (and justified). When libnds was created there was no standard yet. The libnds header change happened after a year or two of creating the initial header, and it was done to match the newly created GBATEK. It was a way to match the standard, not to create a new one. In any case, this isn't a productive discussion, so I'll stop it there as well. |
I've seen "prio" used in various spaces as an abbreviation for "priority". The earliest I remember is NerdTracker II, a 25+-year-old music composition tool targeting the NES sound chip. |
I don't see the point of creating a public archive, because if something is going to never change again, why allocate so much space to it? A user who decides that they stop using We're doing our best to bring improvements to the most users possible, in the least painful way. That is, better names to potentially everyone who's being linked to this repo, with a compat layer if necessary. I agree with "PRIO" being a better shortening of "PRIORITY" than "PRI", which sounds more like "PRINT" to me. As for the LCDC constants, it's rather unfortunate that they need to be prefixed at all, because that's where a third of their length is coming from. But if you'll allow me a suggestion that'll likely raise some pitchforks, and leverages a rarely-seen RGBASM feature: ld a, LCDC_OFF | LCDC_WIN_9800 | LCDC_WINDOW_OFF | \
LCDC_BLOCK21 | LCDC_BG_9800 | LCDC_BG_OFF | \
LCDC_OBJ_16 | LCDC_OBJ_OFF | LCDC_PRIORITY_OFF People do this in C, why not in assembly? As for the shared OAM/attr constants, I think they should be duplicated; mostly because then we can give them each proper prefixes ( |
Well, public archive or not, I think the right approach would be either to have two repositories, or to have
As I've said before, this isn't really an improvement, but a complete rewrite that has very little to do with the old version that people are using. Whether that's an improvement or not is up to the users to decide, but any user that downloads This doesn't look to me like a good approach to updating things. Nobody expects an update that suddenly breaks everything, this doesn't look "the least painful way" at all. The least painful way is to leave the old header as it is and create a new one with any improvements you want. And you can have a compatibility header or whatever you want to make the migration easier, of course. |
Exactly, thank you. <3
Major version increments explicitly say "things will break", and if people don't want to change any of them, a single back-compat Besides, as you noted in #51, "people don't try to update everything to match the last "right way" of doing things." Old projects were never going to update this file anyway, and new ones will get the benefits without needing to learn that a new repo exists.
Little is being actually "rewritten"; most names are only affected because of changing the And yes, since constants would change anyway, this seemed like an ideal opportunity to improve some of the actual names, like
We do have a compatibility header. The only controversy is over whether the improvements get to be called "hardware.inc 5.0" (which would not even need an update to Pan Docs, it doesn't mention the non-official bit/mask constants anywhere) or "hardware2.inc 1.0" (which has no community mindshare and would do worse at making these improvements useful in practice). Nobody is going to grab a new major version of hardware.inc for their old project expecting zero breakage, or at least, nobody should by the semver promise it's always made and the
Again, this does not change register names. Emulators and Pan Docs will not be affected. hardware.inc already defined some aliases for official registers (e.g. |
Thank you @pinobatch! The "prio" abbreviation appears pretty common+Googlable in other contexts, so we've switched to that. :) I also shortened ; old: 133 chars
ld a, LCDCF_OFF | LCDCF_WIN9800 | LCDCF_WINOFF | LCDCF_BLK21 | LCDCF_BG9800 | LCDCF_OBJ16 | LCDCF_OBJOFF | LCDCF_BGOFF | LCDCF_PRIOFF
; new: 134 chars
ld a, LCDC_OFF | LCDC_WIN_9800 | LCDC_WIN_OFF | LCDC_BLOCK21 | LCDC_BG_9800 | LCDC_OBJ_16 | LCDC_OBJ_OFF | LCDC_BG_OFF | LCDC_PRIO_OFF (As for why I bothered adding underscores, it's because words are hard to distinguish in same-case names without any separator. For example...) |
I think forking these changes off into a separate repo would be a mistake in the long-term. If these changes were forked off into a new repo, it seems to me that there are only two possibilities; either the original repo would be archived (in which case, what was the point of forking?) or the two repos would eventually diverge, which would certainly add to the mess. Letting the two repos diverge would only even possibly be a good idea if there were enough people that actually believed that the current 4.0 names are better, but I haven't seen anyone even pretend to argue that. So, if we don't put these changes in a new repo, then the only question left to answer is: Do we stick with the current (genuinely awful, IMHO) bit/flag naming convention forever? Or do we replace it with a much, much better naming convention? If we merge this, then all new projects will just start with 5.0 and get all the gains. Existing projects can just stick with 4.0 (it isn't broken; it works just fine) or they could update to 5.0 easily by using hardware_compat.inc or they could update to 5.0 and update their source code to use the new, better names. Updating their source code wouldn't even be that much trouble because the proposed changes in this PR aren't that radical; they're quite straightforward and systematic (and this PR isn't changing everything). But again, this is a choice; they aren't forced to make all the updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more critiques from me after this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with all of Dannye's comments diverging the repos. This is a massive improvement as-is... Most users will continue using the hardware.inc they are using, or can redownload older versions if the prefer. The hardware_compat file will also help with older repos. New repos will benefit greatly from the better names which everyone seems to agree are actually better. The only concern was "breakage"...
I don't know if this is the right thing to do, but I feel like there's a strong push for this change. Maybe I'll come to regret this, but we won't know if we don't try—and if strong pushback against this appears from the wider community, then we also have the option of reverting the changes, one way or another. Therefore, merging this. Alea jacta est. |
Thank you! I'll start using this in pret soon. :) (Note that the only changed constants here which match what pret already had, are |
Update the date in HISTORY.md before merging!
Fixes #26
This is a significant breaking change to the traditional hardware.inc names:
REGB_NAME
are changed toB_REG_NAME
(a prefix is easier to read than an infix)REGF_NAME
are changed toREG_NAME
(there's no ambiguity, and some flags and values already lacked anF
, e.g.AUD3LEVEL_*
orAUDENA_OFF/ON
)SCRN
toSCREEN
,HBL
toHBLANK
,X
toWIDTH
, etc, in order to still be short but also more recognizableDPAD
" is now "CTRL_PAD
" because that's the official name for itWhat has not changed:
rNAME
s. Even the aliases likerP1
forrJOYP
are still supported, not deprecated.rSMBK
alias forrSVBK
/rWBK
is deprecated, sincerWBK
is the new preferred alias andrSMBK
was an invention from hardware.inc 3.0.CTRL_PAD
instead ofDPAD
.)Since this change affects so many constants, hardware_compat.inc is provided for backwards-compatibility. That's also where constants deprecated by previous versions have been moved to -- I didn't just remove them, since if someone just wants a drop-in fix for their old project, they might still use deprecated things.
A quick two-command sanity check:
rgbasm hardware.inc
rgbasm -P hardware.inc hardware_compat.inc