pop'n music 15 - 18 implemented - [merged] #219

Closed icex2 closed 1 year ago

In GitLab by @33c17f40 on May 30, 2022, 03:46

_Merges popnsupport -> master

Summary

Adds support for pop'n music 15 to 18 in bt5.

Description

popnhook1 for pop'n 15 - 18 has been added
popnio has been added
inject.exe has new syntax for loading hook DLLs, real.dll=hook.dll. This will trigger an early IAT hook where it will load the EXE suspended without resolving imports, replace the reference to real.dll in the import table with hook.dll, and then resolve everything before launching. This allows for ezusb.dll to be hooked properly.
launcher.exe also has a new early IAT hook feature now. Use -I real.dll=hook.dll. The idea is the same as described above for inject.exe.
Updated ezusb constant namings based on what is visible in ezusb.dll's debug statements.

The launcher.exe implementation of early IAT hooking means that someone can implement popnhook2.dll for 19 and above. I have tried pop'n music Sunny Park using a modified version of popnhook1 and it seems to work to some degree: the I/O check and security check returns OK which means the ezusb hooking used in popnhook1 is also working for the later games using launcher.exe -I ezusb.dll=ezusb2-popn-shim.dll .... The process is rather invasive (manually resolving all imports means more chances to fail) so it has been implemented in such a way that the launcher will work the same as it has before as long as -I isn't specified.

One questionable thing I am not confident about is the texture_usage_fix hack flag I added in the conf. As the comment says, pop'n music 16 will work in Windows XP without the flag being set, but the game will immediately crash on later OSes without the flag being set in my experience. No other games had this issue in my experience. Enabling it in other games doesn't seem to have any negative effects.

Related Issue

https://dev.s-ul.net/djhackers/bemanitools/-/issues/28

How Has This Been Tested?

By launching and playing pop'n music 15, 16, 17, and 18.

Offline as well as network + card in has been tested for all games on Windows XP, Windows 10, and Windows 11.

Checklist

[ ] Implemented (unit) test(s) which prove that the introduced changes are working as expected.
Tested with the following games:
- [x] ... pop'n music 15
- [x] ... pop'n music 16
- [x] ... pop'n music 17
- [x] ... pop'n music 18
[x] Followed the developer (style) guidelines.
[x] Updated existing doc of or add new doc to README file(s).
[x] Updated development documentation.

In GitLab by @33c17f40 on May 30, 2022, 03:55

The ezusb changes could affect IIDX so if someone could please test IIDX games (any of the roundplug games) to make sure they still work properly, it would be appreciated. I gave some games a quick test while working on this and it still seemed to work but I couldn't test every roundplug IIDX game.

In GitLab by @33c17f40 on May 30, 2022, 03:59

added 7 commits

abf40d02...ccfe6f79 - 6 commits from branch djhackers:master
d8f5828d - pop'n music 15 - 18 implemented

Compare with previous version

In GitLab by @33c17f40 on May 30, 2022, 13:08

added 1 commit

0fa3bf10 - Remove old test code

Compare with previous version

In GitLab by @33c17f40 on May 30, 2022, 13:24

added 1 commit

fe0beded - Small cleanup

Compare with previous version

Wouldn't ezusb-popn-shim.dll be sufficient here? Is the 2 in the name denoting anything specific?

I am wondering if we need to expose this as a configurable setting. If you always apply the fix, does this break anything on the other games and under Windows XP? If not, I would prefer turning this into a hardcoded solution and have less configurable options exposed.

I would remove this line as you assume that one hook will likely cover all games. Not saying it cannot, but we have seen unpleasent surprises in the past and had to split into more hook DLLs than we wanted. Can be added once the hook is available.

What about lights?

Dongle 1-5? Wondering how you figured that out.

Hm, there are a lot of iidx references in the types still in the code. I get the idea that they apparently shared a lot of functionality and, with being the same, re-using code makes sense. However, it's somewhat odd to read now and it introduces a weird semantic dependency to the iidx stuff. Not sure what's the best approach here, but I would call "when in doubt, copy-paste first, consolidate leter when everything cleared up?".

What do you think?

Nvm, I think it makes sense to keep this as it denotes BT5's semantics of ezusb(1) vs. ezusb2. Forgot popn uses the ezusb (FX) 2 board.

@tau might be something to upstream into mainline capnhook?

Another indicator that iidx stuff leaks into popn and creates some unpleasent coupling. To keep things simple, I would favor copy-paste the stuff you need for popn for now into its own module/namespace. Consolidation, if necessary and a good idea, can follow later.

We have these documented in Modules.mk at the top. Might want to move that to a proper readme file in docs overall and add the ones you have here.

* [pop'n music](doc/popnhook/README.md)

In GitLab by @33c17f40 on Jun 6, 2022, 01:04

Commented on README.md line 70

changed this line in version 5 of the diff

In GitLab by @33c17f40 on Jun 6, 2022, 01:04

added 1 commit

0afcd743 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

In GitLab by @33c17f40 on Jun 6, 2022, 01:12

Commented on dist/popn/gamestart-15.bat line 15

Yes, that's exactly the reasoning I chose for the name ezusb2 instead of ezusb.

In GitLab by @33c17f40 on Jun 6, 2022, 01:15

Commented on dist/popn/popnhook-16.conf line 23

None of the other games need it so I'm ok with making it a hardcoded solution instead, but I do not want it enabled for all games unless people find that the other games don't work without it (in my testing on WinXP, Win10, and Win11 that wasn't the case). At best it does nothing and at worst it could have unintended side effects. I think it should be easy to conditionally enable it based on the mcode though so that's not a big deal.

In GitLab by @33c17f40 on Jun 6, 2022, 01:17

Commented on doc/popnhook/README.md line 19

changed this line in version 6 of the diff

In GitLab by @33c17f40 on Jun 6, 2022, 01:17

added 1 commit

aaee2f16 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

In GitLab by @33c17f40 on Jun 6, 2022, 01:37

Commented on src/main/ezusb-iidx/secplug-cmd.h line 36

That's just how the code works out in ezusb.dll.

Using pop'n 16's ezusb.dll for a concrete example. There is a large message handler at 0x1000C0E0. At the top of the functions it calls you'll see stuff like (using IDA's decompilation as an example, because it's faster)

  *(*(v4 + 28) + 2) = 1; // node ID, 1 = EZUSB_IIDX_MSG_NODE_SECURITY_PLUG
  *(*(v4 + 28) + 3) = 3; // command ID, 3 = EZUSB_IIDX_SECPLUG_CMD_V2_WRITE_DATA

That's an easy way to know exactly what node and command you're looking at out of the message handlers. From there, you can usually find debug messages/errors. For example, node ID 1 command ID 3's error message says "[Security] ライトエラー" ("[Security] Write error"). So you know it's the write command. Similarly, node ID 1 command ID 2's error message says "[Security] リードエラー" ("[Security] Read error").

This part of the message handler corresponds to the 5 dongle messages.

  if ( this[6] > 11 )
  {
    *(this[7] + 88) = result;
    *(this[7] + 92) = sub_10007C50;
  }
  if ( this[6] > 12 )
  {
    *(this[7] + 96) = result;
    *(this[7] + 100) = sub_10007C70;
  }
  if ( this[6] > 13 )
  {
    *(this[7] + 104) = result;
    *(this[7] + 108) = sub_10007C90;
  }
  if ( this[6] > 14 )
  {
    *(this[7] + 112) = result;
    *(this[7] + 116) = sub_10007CB0;
  }
  if ( this[6] > 15 )
  {
    *(this[7] + 120) = result;
    *(this[7] + 124) = sub_10007CD0;
  }

If you go into each function they look like this:

int __cdecl sub_10007C50(int a1)
{
  return sub_10007A00(a1, *(dword_1003000C + 12));
}

int __cdecl sub_10007C70(int a1)
{
  return sub_10007A00(1, a1, *(dword_1003000C + 12));
}

int __cdecl sub_10007C90(int a1)
{
  return sub_10007A00(2, a1, *(dword_1003000C + 12));
}

int __cdecl sub_10007CB0(int a1)
{
  return sub_10007A00(3, a1, *(dword_1003000C + 12));
}

int __cdecl sub_10007CD0(int a1)
{
  return sub_10007A00(4, a1, *(dword_1003000C + 12));
}

They all are calling the same sub_10007A00 with the values 0-4. The 0-4 value is used to set the node ID/command ID as such:

  switch ( a1 )
  {
    case 0:
      v6 = *(a2 + 12);
      *(*(v6 + 28) + 2) = 1;
      *(*(v6 + 28) + 3) = 7;
      goto LABEL_9;
    case 1:
      v7 = *(a2 + 12);
      *(*(v7 + 28) + 2) = 1;
      *(*(v7 + 28) + 3) = 8;
      goto LABEL_9;
    case 2:
      v8 = *(a2 + 12);
      *(*(v8 + 28) + 2) = 1;
      *(*(v8 + 28) + 3) = 9;
      goto LABEL_9;
    case 3:
      v9 = *(a2 + 12);
      *(*(v9 + 28) + 2) = 1;
      *(*(v9 + 28) + 3) = 10;
      goto LABEL_9;
    case 4:
      v10 = *(a2 + 12);
      *(*(v10 + 28) + 2) = 1;
      *(*(v10 + 28) + 3) = 11;
...

The shared function sub_10007A00 has a debug error message of "[Security] セレクトエラー" ("[Security] Select error") so it's the dongle/whatever selection function.

So it's safe to say that there are 5 dongle commands in total. How they differ exactly is harder to understand because the code is the exact same between all 5 of the commands, except for the command ID being assigned. And the game itself expects a white dongle or black dongle to be returned depending on what slot it reads. IIDX is lenient here but pop'n is picky and will only work with the one specific configuration.

In GitLab by @33c17f40 on Jun 6, 2022, 01:45

Commented on src/main/bemanitools/popnio.h line 86

That's true I guess. I never do light emulation because I have no way to test or use it. I'll see about at least exposing it.

In GitLab by @33c17f40 on Jun 6, 2022, 01:49

Commented on src/main/ezusb2-popn-emu/msg.c line 42

I already discussed this on Discord but there's way too much code here for me to comfortably just copypaste it all. I personally question if any of the nodes really even need to be game-specific.

In GitLab by @33c17f40 on Jun 6, 2022, 01:52

Commented on src/main/popnhook1/dllmain.c line 6

changed this line in version 7 of the diff

In GitLab by @33c17f40 on Jun 6, 2022, 01:52

added 1 commit

e77fe108 - Move avs versions to Module.mk

Compare with previous version

In GitLab by @33c17f40 on Jun 6, 2022, 01:59

Commented on dist/popn/popnhook-16.conf line 23

changed this line in version 8 of the diff

In GitLab by @33c17f40 on Jun 6, 2022, 01:59

added 1 commit

e37ee88f - Hide texture_usage_fix setting

Compare with previous version

In GitLab by @33c17f40 on Jun 6, 2022, 03:39

added 1 commit

ee72ec1f - Use constant to make code clearer

Compare with previous version

I was actually wondering if you need a feature toggle for that to begin with. I guess that works, but if it doesn't hurt to have that texture fix on for all versions supported by popnhook1, that would streamline the code further by removing that one condition. If that isn't an option, this is fine as is.

I personally question if any of the nodes really even need to be game-specific.

That's a fair point. The changes right now don't break anything, but create some sort of tech-debt that needs to be addressed rather sooner than later. Considering the high value of this MR, I would not make this a blocker for merging as it can be followed up in a separate refactoring MR.

Getting another opinion since he also worked a lot on iidx stuff: What do you think @xyen?

I suggest that we should have at least the API "completed" as we do not have means for versioning on an API level right now (which sucks). I see that once we create an actual release package, people will very quickly create their own implementations of popnio.

Can you come up with a meaningful function signature that you just stub for now in the default popnio implementation?

In GitLab by @xyen on Jun 7, 2022, 23:38

Commented on src/main/ezusb2-popn-emu/msg.c line 42

I'm fine with leaving it as-is, and merging / renaming the node later

In GitLab by @33c17f40 on Jun 8, 2022, 24:33

added 33 commits

ee72ec1f...4c0d53b0 - 32 commits from branch djhackers:master
deb7f958 - Merge branch 'master' into popn_support

Compare with previous version

In GitLab by @33c17f40 on Jun 8, 2022, 24:39

added 7 commits

c10d0e41 - pop'n music 15 - 18 implemented
d1ab369e - Remove old test code
f851fa89 - Small cleanup
047f4eed - Apply 1 suggestion(s) to 1 file(s)
febfea95 - Move avs versions to Module.mk
4611cc97 - Hide texture_usage_fix setting
556a08d7 - Use constant to make code clearer

Compare with previous version

In GitLab by @33c17f40 on Jun 8, 2022, 24:49

Commented on src/main/popnhook1/dllmain.c line 124

I can't stop you guys from changing it after it's committed in the future but I do not want to make this an always on patch if I can help it. It's a hack that I don't even really want in the first place and only fixes the one specific game. Applying it to all games could have unintended side effects.

In GitLab by @33c17f40 on Jun 8, 2022, 01:41

Commented on src/main/popnhook1/dllmain.c line 124

changed this line in version 12 of the diff

In GitLab by @33c17f40 on Jun 8, 2022, 01:41

added 1 commit

e0256754 - Add support for pop'n lights

Compare with previous version

In GitLab by @33c17f40 on Jun 8, 2022, 01:43

Commented on src/main/bemanitools/popnio.h line 86

I implemented lights to the best of my ability. I can't test if it works properly when configured but the bits and such I documented in the operator menu also match what was documented in https://dev.s-ul.net/djhackers/bemanitools/-/issues/28#note_7140. Coin blocker and counter aren't configurable I think so I stubbed those.

In GitLab by @33c17f40 on Jun 8, 2022, 01:45

added 1 commit

2f804ec5 - Use memcmp instead of strcmp

Compare with previous version

Ok, np. I definintely trust your judgement on whether this might become an issue or not. Let's keep your solution.

Copy-paste mistake? lights as a parameter doesn't look meaningful. Same goes for the function name popn_io_set_coin_blocker_lights. What about void popn_io_set_coin_blocker(boolean enable); as this addresses only the coin blocker to begin with? Same for coin counter function below

In GitLab by @33c17f40 on Jun 11, 2022, 24:33

Commented on src/main/bemanitools/popnio.h line 110

changed this line in version 14 of the diff

In GitLab by @33c17f40 on Jun 11, 2022, 24:33

added 1 commit

d08f341f - Refactor coin lights

Compare with previous version

In GitLab by @33c17f40 on Jun 11, 2022, 24:35

Commented on src/main/bemanitools/popnio.h line 110

It wasn't a copypaste mistake. The values are 4 bits wide and I didn't know (still don't really) if something can change depending on what bits are set. I just did a quick check of the code and it always seems to write 0xf to those fields, but without someone doing hardware testing I can't say if it could be something besides 0xf.

It's dealing with lights so I'd prefer not to remove the word "light" from the function names so it's clear. The values are being written to usbLamp after all.

Fair points, thanks for explaining and lgtm now.

In GitLab by @33c17f40 on Jun 11, 2022, 24:42

resolved all threads

approved this merge request

djhackersdev / bemanitools