pret / pokecrystal

Disassembly of Pokémon Crystal
https://pret.github.io/pokecrystal/
2.1k stars 803 forks source link

Fix coordinate macros #444

Closed mid-kid closed 6 years ago

mid-kid commented 6 years ago

There seems to be inconsistency in the order of x and y arguments in macros and related data. Right now, hlcoord's arguments are in the x, y order, while the lb bc macro that is often used next to it, is in the height, width order.

Since menudataheaders all specify their coordinates through the y, x format, I propose changing hlcoord to do the same. This will also make it consistent with lb bc's height, width format.

Apparently, there's other (script) macros that deal with the same problem. These should also be changed to follow the y, x and height, width format.

This is all to avoid confusion when jumping around different parts of the code. If we can make everything consistent in this way, people will have an easier time getting their coordinates right on the first attempt.

roukaour commented 6 years ago

The script definition macros consistently use y, x: warp_def, coord_event, bg_event, object_event. But the command macros consistently use x, y: warp, moveobject, changeblock, warpfacing.

I would expect coordinates and dimensions to either reflect whatever internal order their context uses (which won't always be consistent), or stick with the Cartesian plane convention x, y (and thus width, height).

roukaour commented 6 years ago

We might also want to leave them as-is for backwards compatibility. When renaming things, it's obvious they've been changed, but swapping two numbers isn't.

mid-kid commented 6 years ago

If leaving the internal representation intact is big enough a reason not to make everything consistent, we can do that, but hlcoord has no clear internal representation and neither does lb bc. These two are way too often used next to eachother to not warrant any consistency. We could either introduce a bcsize macro or swap hlcoord around.

The reason I proposed y, x is because that's the only internal representation I've been able to find not hidden behind any macro, and because it's consistently used in non-script data.

aaaaaa123456789 commented 6 years ago

I personally find y, x values extremely confusing and would be glad to see them vanish.

aaaaaa123456789 commented 6 years ago

Also, on lb, the registers do have an internal representation; it's bc, not cb. Keep in mind that lb is used to simultaneously load a register pair — it is not related to coordinates.

mid-kid commented 6 years ago

@aaaaaa123456789 Kind of, but not really. How GF actually loaded them depends on the endianness they settled on. Also, while y, x is confusing at first, at least it's consistent, which would make it really easy to get used to and remember, only being a bit of a problem at the beginning. If we go for x, y, we're going to need macros for menudataheaders and lb bc, just to swap those two around - not really something a lot of people are going to be happy with.

aaaaaa123456789 commented 6 years ago

Consider code completely unrelated to coordinates. If you first saw lb de, 100, 250, would you expect it to do d = 100, e = 250 or the other way around? Macros that behave in awkward and confusing ways are a bad idea.

Also, consistent as it may be, y, x is inconsistent with common usage literally everywhere else (i.e., not in the repo). It's the same reason why AT&T style for x86 assembly is almost universally hated, despite it's very consistent.

(For the uninformed: Intel: mov eax, 20; AT&T: movl 20, %eax.)

mid-kid commented 6 years ago

@aaaaaa123456789 I get the lb thing, the point I'm trying to make is that we don't really know if that's how GF actually used them. Maybe they just ld de, $yyxx instead of the fancy macros we use, we don't really know.

aaaaaa123456789 commented 6 years ago

@mid-kid I'm really interested in that ld be, $1234 instruction :)

That being said, I doubt they'd do it without some sort of macro. ld bc, $1214 and ld bc, 4628 both seem notoriously inferior to lb bc, SCREEN_HEIGHT, SCREEN_WIDTH.

mid-kid commented 6 years ago

Besides, y, x usage is not as inconsistent with the rest of the world as you're talking about, though. In a lot of graphics-related code the y is the first thing you're going to want to know, before loading the x. In z80, specifically, it's much more efficient to put the values in the order you're going to use them, so unless they used a macro to swap both around, they might have actually settled for specifying everything except apparently script arguments in the y, x format.

I would be fine with x, y too, of course. It feels more natural, but I'd rather avoid even more needless macros than what we already have.

roukaour commented 6 years ago

Map scripts are now standardized on x, y as of PR #445 . I haven't changed the code macros like hlcoord.

mid-kid commented 6 years ago

We still require a macro for menu headers and lb cb.

roukaour commented 6 years ago

PR #450 defines hldims, bcdims, dedims, and dims analogous to hlcoord, bccoord, decoord, and coord, with the coorrect (width height) argument order. They are not yet used anywhere though.

roukaour commented 6 years ago

So a (width, height) dims macro has been rejected and #440 encompasses menu data coordinates; everything else mentioned in this issue is completed. Ready to close?