search

gbdev / hardware.inc

RGBDS include file for Game Boy hardware definitions
Creative Commons Zero v1.0 Universal
119 stars 21 forks source link

Add several more flags, bit numbers, and offset constants #14

Closed rondnelson99 closed 3 years ago

rondnelson99 commented 3 years ago

Solves #13. Am I doing this right? This is my first time contributing to someone else's project.

ISSOtm commented 3 years ago

Please also update the revision number, and add a line to the history explaining the changes.

rondnelson99 commented 3 years ago

Cool. I can add those, but there were a couple of other things that I think would also be worthwhile to add. One is adding flag definitions for rSC and rIF, since they are both missing these. I would also add bit numbers to them as there could be utility in using the bit instruction with them. Another thing I feel would be useful would be defining WX_OFFSET to 7 to account for the fact that 7 must be added to a screen position to calculate a rWX position.

Are there additions worthwhile? If so, should I just commit them to my fork or should I make separate pull requests for them?

rondnelson99 commented 3 years ago

Well, I went ahead and commited those changes. I can always revert them if they're undesirable. I'm not too sure about the rIF flags myself since they're just copy-pasted from the rIE flags.

rondnelson99 commented 3 years ago

I've additionally added offset constants for OAM X and Y position in the same vein as the WX_OFFSET I committed earlier.

avivace commented 3 years ago

Why are we suggesting both OFS and OFFSET? @ISSOtm

ISSOtm commented 3 years ago

It seemed to me that WX_OFFSET was still short enough that fully spelling it out was OK, so I elected not to change it. (I'm not suggesting to correct it, only to add a blank line between it and the register constant, as is done with other "associated constants" throughout the file.)

If you think that's inconsistent, we can change it to WX_OFS, I suppose.

avivace commented 3 years ago

It seemed to me that WX_OFFSET was still short enough that fully spelling it out was OK, so I elected not to change it. (I'm not suggesting to correct it, only to add a blank line between it and the register constant, as is done with other "associated constants" throughout the file.)

If you think that's inconsistent, we can change it to WX_OFS, I suppose.

You did correct some instances of OFFSET to OFS while you allowed others. I think consistency here should be a priority (then length of the names).

ISSOtm commented 3 years ago

Edited accordingly.

avivace commented 3 years ago

Edited accordingly.

Great, thanks!