Skip to content
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

SCI: Add patch opcodes enum for better readability #1716

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@OmerMor
Copy link
Contributor

commented Jun 27, 2019

The motivation is to allow for nicer script patches with opcode names instead of "magic" numbers.

Using the new enum, 0x39, 0x7a, would become OP_0x39_PUSHI, 0x7a,.

This change is backward compatible, so patches could still maintain their magic opcode numbers. This would allow for incremental refactoring of existing patches, while writing new ones with the opcode names.

I prefixed the opcode names with their numeric value for 2 reasons:

  1. To differentiate between the opcodes with LSB = 0 and 1.
  2. For easier conversion of SCI assembly to patches. When writing the patch, the source is written in numeric form. It would be easy to start writing e.g. OP_0x81_ and auto-complete it into OP_0x81_LAG.
@bluegr

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I’m a bit undecided on this one. It does help with readability, but the existing script patches will have to be adapted. Creating a script that would mass convert the existing patches would be preferable, instead of leaving this task to be done at some point in the future.

Could you please help by working on such a script?

@sluicebox

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

I'm on the fence too.

What's weird is that logically, this is a natural improvement that should make things more readable, but SCI script patches are a weird medium. It would be a bit of work to convert them all and I don't think the benefits are significant, while there are downsides.

Pros to enum'in:

  • Patch authors can use IDE autocomplete to write opcodes
  • Patch readers can see the mnemonic when looking at the left column

Pros to just hex'in:

  • Left column matches disassembly format from tools I rely on (ScummVM, SCI Companion, SCI Viewer)
  • Left column takes fewer characters, more room for right column (where readability lives)
  • Right column has mnemonics and annotations right next to each other, no need to eye dart to the inhumane left column
  • Left column opcodes naturally align since always 4 chars
  • It's not a big deal to keep the comments in sync relative to the hard parts of writing these patches (except when I blow it, in which case @bluegr catches it)
  • Since there are multiple opcodes for the same operation, you need to know and read the hex codes anyway
  • The worms have already ate into my brain and I type the opcodes by memory (sigh)

As a patch author, I think this would add another step for me to ensure readability. I'd OCD over keeping the opcode column space-aligned so that the operand column was left aligned, and this would push the right column off even farther, which is bad because that's the one with a shot at readability. Would the existing mnemonics leave the right column once they'd been enum'd? I sweat a lot of details to make my patches holistically readable and I suspect changing the structure I took into account at the time will decrease that.

A while ago someone reformatted a ton of patch comments, some of which were needed, most of which were not, and I responded with a curter comment than warranted because I took offense, but I should have instead taken that opportunity to lay out my thoughts.

I recently had to read someone else's mid-sized patch to port it to a different game version. I didn't know anything about the bug or script prior. WOW it was hard, and it was a well documented patch. It confirmed what I suspected, that the patch code itself (two arrays of bytes) is inherently unreadable, even with a strong narrative and line-level annotations, unless you personally dive in with the original disassembly and learn the script. That's okay! Done right, these are write-once affairs with (hopefully!) entertaining narratives. Unlike normal code the goal is for them to never be maintained.

Come to think of it my one formatting wish is the opposite, for the left column's macro names to be shorter, like PATCH_GETORIGINALUINT16ADJUST(), and that's just because I want more room for the right.

If these enums help patches get written, I'm all for it. I'm uncomfortable with converting the existing ones to it, because it means monkeying with the existing comments, which only the original author had the context to express clearly, and I defer to them. I'm okay with the enums being optional, but that doesn't sound right either because then there's more than one way.

Despite being a parade of c++ word arrays, I'd wager that script_patches.cpp is the most entertaining part of the ScummVM codebase, and our best chance of bringing down the github UI should it ever start to show Terminator-like inclinations.

@bluegr

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

The resulting code is nice, but the comments now duplicate the patch:

OP_0x39_PUSHI, 0x7a,                 // pushi 7a <-- initialization code when walking automatically
OP_0x78_PUSH1,                       // push1
OP_0x7a_PUSH2,                       // push2
OP_0x38_PUSHI, SIG_UINT16(0x00a9),   // pushi 00a9 - script 169
OP_0x78_PUSH1,                       // push1
OP_0x43_CALLK, 0x02, 0x04,           // callk ScriptID
OP_0x36_PUSH,                        // push
OP_0x81_LAG,   0x00,                 // lag global[0]
OP_0x4a_SEND,  0x06,                 // send 06
OP_0x32_JMP,   SIG_UINT16(0x0520),   // jmp [end of fawaz::handleEvent]

Thing is, we need to keep one consistent way of writing script patches. Since we do have comments in all script patches, adding an enum is superfluous in existing patches. I can think of this being useful in new script patches, where the enum will substitute the corresponding comment. But then, we do not have a unified way of writing script patches.

Thus, we should either merge this and convert all the current script patches to use the enums, or not merge this, and keep writing the patches the way we do now.

@Kawa-oneechan

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I'm with @sluicebox on this. Except for the bit about the worms.

@lskovlun

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

For opcodes like callk, you're going to have to put a comment anyway (so it is clear which kernel function is being called). Lots of other opcodes benefit from a comment in any case. I don't see how this helps.

@OmerMor

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

I thought the code could be sprinkled with more consts and the comments could be restricted to higher level explanations, a bit like this:

  OP_0x72_LOFSA, SIG_MAGICDWORD, SIG_UINT16(fawaz), // <-- start of proper initializion code
  OP_0xa1_SAG,   0xb9,                 // sag global[b9h]
  SIG_ADDTOOFFSET(+571),               // ...
  OP_0x39_PUSHI, 0x7a,                 // <-- initialization code when walking automatically
  OP_0x78_PUSH1,                       // 
  OP_0x7a_PUSH2,                       // 
  OP_0x38_PUSHI, SIG_UINT16(0x00a9),   // pushi 00a9 - script 169
  OP_0x78_PUSH1,                       // 
  OP_0x43_CALLK, K_ScriptID, 0x04,     // (ScriptID 169 4)
  OP_0x36_PUSH,                        // 
  OP_0x81_LAG,   0x00,                 // lag global[0]
  OP_0x4a_SEND,  0x06,                 // 
  OP_0x32_JMP,   SIG_UINT16(0x0520),   // jmp [end of fawaz::handleEvent]
  SIG_END
};```

My main pain point with existing patches was the abundance of magic numbers.
However I can see how my version is not clearer than the hand-written assembly in the comments - so it's not a real win.
It could make life a bit easier on patch writing through the use of auto-complete, but since the intended audience is doing fine without it, there's not much point for this either.
I'll go ahead and close this PR now then.
Thanks for the review!

@OmerMor OmerMor closed this Jul 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.