Open joeldipops opened 5 years ago
First, let's get one thing straight. I see the intention and effort that went into this. I believe it was also the case to the currently three persons who left you a thumbs up. [EDIT: forgot to mention this] Some of your work, especially the addresses, would be good to integrate into hardware.inc. However, I have two problems with the project.
The first one is that it's really unoptimized - this on its own is, imo, enough to reject it on the awesome list. Instead, I've sent a couple PRs to hopefully address this.
The second problem is basically that I'm firmly opposed to pseudo-op macros. This can be broken down into three sub-problems: a sharp loss in readability, being a major hindrance to optimization, and just being a huge footgun. Below is a more detailed explanation of each of those.
Readability The instructions are part of the base instruction set everyone knows about. They're what you can expect anyone to know. These macros, however, are not. If they become part of a codebase, it requires someone to find the file they are defined in (which is not always easy, especially if it's poorly structured), read the macros, and remember them.
It's fine, since the macros are simple... but then, why have the macros at all? Is this worth requiring additional knowledge from maintainers?
cpAny: macro
ld A, \1
cp \2
endm
Why not just write the two lines? They're small, they're not confusing.
Further, let's assume you want to OR two values from memory. Alright:
orAny b, c
But you find you also need to or
with d
. What would you do?
orAny b, c
or d
is a good option, but it's not consistent...
orAny b, c
orAny a, d
is very consistent, but it generates a ld a, a
instruction, which should be never exist in ASM code.
What about special-casing a
as a first argument, and skip the first ld a, \1
? Then you have just reinvented the or
instruction, just given it a different name. And, at this point, you're not writing ASM anymore, you are making your own language.
Optimization Here is an example. On a surface level, this seems fine; but, the loop body expands to
add a, c
ld l, a
ld a, h
adc a, 0
ld h, a
which is known to be optimizable to the following (1 byte, 1 cycle):
add a, c
ld l, a
adc a, h
sub l
ld h, a
My point is, the macros are a major barrier to optimization, because they group some instructions that could otherwise be analyzed as a whole, split, and optimized.
Another important thing is that they also constrain the way the code is written: resAny
resets a bit in a byte in memory, but there's another way to do it, which is to use ld hl, addr ; res N, [hl]
. This method is atomical, which might be mandatory in some cases (eg. interfacing with an interrupt handler, as I'm currently doing in my own game).
Further, to prevent side effects, your mult
macro I linked to pushes BC and does a xor a
to "make flags consistent". This is a 8-cycle penalty, which is completely gratuitous (especially the xor a
- it doesn't matter if the flags are inconsistent, just don't rely on them!) and sometimes completely unnecessary:
ld a, [wSelectedBagSlot]
ld c, BAG_SLOT_SIZE
mult a, c
ld bc, wBagContents ; BC is trashed anyways
add hl, bc
Footgun potential Let's take this innocuous snippet of code.
orAny [addr], b ; `ld a, [addr]` then `or b`
It's my first time seeing it, but I read the comment and the name (but not the code, because comments should be enough). I deduce that it's performing a OR, which I know is a commutative operation, so I can do this!
orAny b, [addr] ; `ld a, b` then `or [addr]`
But it doesn't compile! Why?
The other huge problem is side effects. There are two possibilities: either you have as little side effects as possible by spamming push
and pop
, which incurs larger and slower code as well as thrashes the stack; or you leave them to the user to work around if they need to, which requires documenting them every time a change is made as well as making sure no mistakes are made.
To sum all this up, I think pseudo-op macros should not be used period, and thus I'm against including them in the awesome list. I'm open for debate, though.
I originally started writing a comment, but I waited because I knew @ISSOtm would write a comment like that one :wink: So I left a :+1: to show my support in the meantime.
His analysis of the library itself is good, and more insightful than what I could make. These are all issues that can be fixed, however (mostly by documenting - side effects of a pseudo-op, for example, aren't any worse than side effects of regular ops, but both obviously need to be documented), and these issues are not mentioned in the summary so I think we can disregard them for the broader discussion:
To sum all this up, I think pseudo-op macros should not be used period, and thus I'm against including them in the awesome list. I'm open for debate, though.
This is a more interesting debate. I don't think personally being against pseudo-ops (which I also am; I'm also against CamelCase!) is grounds for not including them on the list. I also wouldn't use C to program a Game Boy game, but we still have lots of C resources on there.
Like C, these pseudo-ops have one important goal: Make programming Game Boy games more accessible. But these pseudo-ops are even better than C: They can make programming Game Boy games in assembly more accessible!
I dislike boilerplate in code. Anything that can remove boilerplate is good, in my book. There's a reason most of us use the same memcopy
and wait_vblank
routines from gbhw.inc
/hardware.inc
and have been for 20 years. These pseudo-ops can abstract away boilerplate for simpler things that we do all the time, like juggling values in and out of a
, and I think that can potentially be very useful.
Now, the magical routines that we replace the boilerplate code with should of course work, and they should be documented well. (People should also know what the actual ops of the GB CPU are before resorting to pseudo-ops, but luckily our list covers that already.) If those two things are true, I see nothing wrong with including them on the list.
To sum all this up, I think pseudo-op macros should not be used period, and thus I'm against including them in the awesome list. I'm open for debate, though.
This is a more interesting debate. I don't think personally being against pseudo-ops (which I also am; I'm also against CamelCase!) is grounds for not including them on the list. I also wouldn't use C to program a Game Boy game, but we still have lots of C resources on there.
I'm making a difference there; I'm against pseudo-ops period, whereas I'm against C in some cases (roughly anything but programming under time constraints, = game jams). That's why I'm OK with C on the awesome list, but not with this.
Like C, these pseudo-ops have one important goal: Make programming Game Boy games more accessible. But these pseudo-ops are even better than C: They can make programming Game Boy games in assembly more accessible!
I'm not sure it makes it more accessible... due to the footgun problem, which newbies will probably not be aware of. In fact, I'd consider pseudo-ops to be harmful and should be avoided for everyone but experienced developers.
I dislike boilerplate in code. Anything that can remove boilerplate is good, in my book. There's a reason most of us use the same
memcopy
andwait_vblank
routines fromgbhw.inc
/hardware.inc
and have been for 20 years. These pseudo-ops can abstract away boilerplate for simpler things that we do all the time, like juggling values in and out ofa
, and I think that can potentially be very useful.Now, the magical routines that we replace the boilerplate code with should of course work, and they should be documented well. (People should also know what the actual ops of the GB CPU are before resorting to pseudo-ops, but luckily our list covers that already.) If those two things are true, I see nothing wrong with including them on the list.
I'm not sure what you call "boilerplate" here.
This is cool!
I'm late to the party but I'd very much like to see this in the list. @ISSOtm observations are on point and he raised some clear issues. I think that documenting and providing context, describing the downsides/inner workings of each routine would be very effective and effectively make this usable in a healthy way.
@joeldipops do you think you can add some documentation to address this points? I've seen @ISSOtm opened a couple of PR on your repository. Can you take a look at them?
Keep us updated!
Yep, I do see ISSOtm's points for a lot of things, and addressing them has been at the back of my mind for a while. Needless to say I disagree that pseudo-ops are 'always' an anti-pattern, but I definitely see the issue with some of these, in particular, ops with effects like and/or/xor.
I don't agree that "at this point, you're not writing ASM anymore, you are making your own language." is a problem. My goal was a clean interface and implementation details can be black-boxed. If I can improve the library's usability, maintain its performance, and reduce its foot-shoot-potential by stuffing it with lots of rgbasm macro-language, I'd like to pursue that. I've started down that path, though I need to check my docs are up to date with it.
As for the specific pull requests that were raised, yep, I've looked at those and incorporated some changes, though there's room for improvement there.
At present I'm working on the N64 side of my main project, so I won't be looking in depth at this until I swing back towards the Game Boy, but if there's any more suggestions/pull requests/bugs in the mean time I'll definitely take a look.
I've reviewed the project and opened issues and PRs on problems. I don't understand why you just closed them without giving a chance for improvement--this isn't what we've done here, for example.
I didn't "just close" anything. I closed pull requests after making some efforts to incorporate their suggestions into my own commits, and opened new issues relating to them where I understand further work needs to be done.
Do we have any update on this, @joeldipops ?
This also looks interesting: https://github.com/joeldipops/TransferBoy
I have made quite a few improvements to it, especially based on your and ISSOtm's advice. There's plenty of room for improvement yet, especially on the documentation side of things. That said I haven't touched it in a while since I've been focusing on TransferBoy and a new baby. Thanks for taking a look.
I have made quite a few improvements to it, especially based on your and ISSOtm's advice.
Great! I really appreciate you sticking with it and working on feedbacks.
a new baby
That's so cool! Congratulations! 😍
I think this basically reduces to a general stance we have to take on Macros/these type of black-boxing
I have been working on a library that I think other Game Boy developers could make use of. It is hosted here https://github.com/joeldipops/gbz80-pseudoOps
I'm spruiking my own work so I don't feel qualified to answer the question "Is it awesome" but I think it fits the three criteria
It must be in a minimal working state; Although I intend for the library to continue to grow and improve, I believe in its current state it is already useful. The 20 or so macros already written are working and reasonably well documented.
It must have a clear purpose or provide something interesting The purpose is to a) polyfill operations that feel like they could exist on the gameboy CPU but don't, for example moving data from one memory address to another
ldAny [LcdControl], LCD_ON
or 16 bit subtractionsub16 HL, BC
b) Make rgbasm code more readable, through the above polyfills and intuitive constant names.It must provide a minimal documentation briefly describing what is the project and how to make use of it. There's a README.md with instructions for including the library and documenting most of the macros, including size, cycle counts and flags affected.
My only concern is that I'm worried I'm the only person that would find this stuff useful. I haven't seen anyone else that dislikes gbhw.inc and it's "rSCY" style naming, and I've completely ignored some advice I've read not to indent asm code as I would an OOP language.