Add ea3_boot hook for legacy IIDX #300

Merged
gar6a9e merged 3 commits from legacy-iidx-network into master 2024-03-25 16:16:47 +03:00
gar6a9e commented 2024-03-04 02:58:22 +03:00 (Migrated from github.com)

Close: https://github.com/djhackersdev/bemanitools/issues/283

Test

Launch and connection to the arcana server, playing with a card, NETWORK CHECK

Windows 11 23H2 (build 22631.3296)

  • IIDX9
  • IIDX10
  • IIDX11
  • IIDX12
  • IIDX13
  • IIDX14
  • IIDX15 (crashed)
    • In my environment, it crashes during launch even with the version 5.48.
    • From the logs of xrpc and servurl, the changes in this PR seem to be working fine.
  • IIDX16
  • IIDX17

Windows 10 1903 (build 18362.356)

The version at the time this comment was committed (Sep 28, 2019): https://github.com/djhackersdev/bemanitools/blob/cbd7720349a51a8a067a939e2f8104f9e2e691bf/src/main/iidxhook-util/eamuse.c#L73-L75

  • IIDX9
  • IIDX10
  • IIDX11
  • IIDX12
  • IIDX13
Close: https://github.com/djhackersdev/bemanitools/issues/283 # Test Launch and connection to the arcana server, playing with a card, NETWORK CHECK ## Windows 11 23H2 (build 22631.3296) - [x] IIDX9 - [x] IIDX10 - [x] IIDX11 - [x] IIDX12 - [x] IIDX13 - [x] IIDX14 - [ ] IIDX15 (crashed) - In my environment, it crashes during launch even with the version 5.48. - From the logs of xrpc and servurl, the changes in this PR seem to be working fine. - [x] IIDX16 - [x] IIDX17 ## Windows 10 1903 (build 18362.356) The version at the time this comment was committed (Sep 28, 2019): https://github.com/djhackersdev/bemanitools/blob/cbd7720349a51a8a067a939e2f8104f9e2e691bf/src/main/iidxhook-util/eamuse.c#L73-L75 - [x] IIDX9 - [x] IIDX10 - [x] IIDX11 - [x] IIDX12 - [x] IIDX13
icex2 commented 2024-03-19 14:05:36 +03:00 (Migrated from github.com)

Thanks for looking into this as a follow-up to our discussion in the linked issue.

Just skimming this briefly, it seems like this is ready to be reviewed, but your PR title still mentions "WIP". Just want to make sure that you consider your work actually finished before I start review. Please let me know if that's the case.

Thanks for looking into this as a follow-up to our discussion in the linked issue. Just skimming this briefly, it seems like this is ready to be reviewed, but your PR title still mentions "WIP". Just want to make sure that you consider your work actually finished before I start review. Please let me know if that's the case.
gar6a9e commented 2024-03-19 15:53:00 +03:00 (Migrated from github.com)

Thanks for looking into this as a follow-up to our discussion in the linked issue.

Just skimming this briefly, it seems like this is ready to be reviewed, but your PR title still mentions "WIP". Just want to make sure that you consider your work actually finished before I start review. Please let me know if that's the case.

The title includes "WIP" because testing is not yet complete, but reviews are welcome.

> Thanks for looking into this as a follow-up to our discussion in the linked issue. > > Just skimming this briefly, it seems like this is ready to be reviewed, but your PR title still mentions "WIP". Just want to make sure that you consider your work actually finished before I start review. Please let me know if that's the case. The title includes "WIP" because testing is not yet complete, but reviews are welcome.
icex2 (Migrated from github.com) reviewed 2024-03-19 20:40:00 +03:00
@@ -0,0 +95,4 @@
{
log_info("Called my_avs_boot");
avs_boot_replace_property_uint32(config, "log/level", 4);
icex2 (Migrated from github.com) commented 2024-03-19 20:40:00 +03:00

suggestion: Can you adjust the log level to align with what's configured for the hook? That would ensure that the log level can be controlled properly, i.e. to reduce output on normal/production operation.

suggestion: Can you adjust the log level to align with what's configured for the hook? That would ensure that the log level can be controlled properly, i.e. to reduce output on normal/production operation.
icex2 (Migrated from github.com) reviewed 2024-03-19 20:42:40 +03:00
@@ -0,0 +132,4 @@
static int my_ea3_boot_avs(struct property_node *config)
{
log_info("Called my_ea3_boot_avs");
insert_eamuse_addr(config);
icex2 (Migrated from github.com) commented 2024-03-19 20:42:39 +03:00

question: Why are you inserting the eamuse addr on avs boot? AVS should not have to deal with ea3 related stuff.

question: Why are you inserting the eamuse addr on avs boot? AVS should not have to deal with ea3 related stuff.
icex2 (Migrated from github.com) reviewed 2024-03-19 20:43:25 +03:00
@@ -0,0 +48,4 @@
{.name = "ea3_boot",
.patch = my_ea3_boot,
.link = (void **) &real_ea3_boot},
};
icex2 (Migrated from github.com) commented 2024-03-19 20:43:25 +03:00

question: Why are there twoea3_boot functions?

question: Why are there two`ea3_boot` functions?
gar6a9e (Migrated from github.com) reviewed 2024-03-19 22:06:25 +03:00
@@ -0,0 +132,4 @@
static int my_ea3_boot_avs(struct property_node *config)
{
log_info("Called my_ea3_boot_avs");
insert_eamuse_addr(config);
gar6a9e (Migrated from github.com) commented 2024-03-19 22:06:25 +03:00

It's because it was modeled after the implementation of popnhook.

https://github.com/djhackersdev/bemanitools/blob/master/src/main/popnhook1/avs-boot.c#L72

It's because it was modeled after the implementation of popnhook. https://github.com/djhackersdev/bemanitools/blob/master/src/main/popnhook1/avs-boot.c#L72
gar6a9e (Migrated from github.com) reviewed 2024-03-19 22:09:40 +03:00
@@ -0,0 +48,4 @@
{.name = "ea3_boot",
.patch = my_ea3_boot,
.link = (void **) &real_ea3_boot},
};
gar6a9e (Migrated from github.com) commented 2024-03-19 22:09:40 +03:00

It's because the implementation of the ea3_boot function varies by game version.

https://github.com/gar6a9e/bemanitools/blob/legacy-iidx-network/src/main/iidxhook3/avs-boot.c#L148

It's because the implementation of the `ea3_boot` function varies by game version. https://github.com/gar6a9e/bemanitools/blob/legacy-iidx-network/src/main/iidxhook3/avs-boot.c#L148
gar6a9e (Migrated from github.com) reviewed 2024-03-19 22:21:35 +03:00
@@ -0,0 +95,4 @@
{
log_info("Called my_avs_boot");
avs_boot_replace_property_uint32(config, "log/level", 4);
gar6a9e (Migrated from github.com) commented 2024-03-19 22:21:35 +03:00

What specific steps should be taken?
For example, making it configurable via conf file?

The reason for hardcoding the log level was to follow the existing implementation.

https://github.com/djhackersdev/bemanitools/blob/master/src/main/jbhook1/avs-boot.c#L118
https://github.com/djhackersdev/bemanitools/blob/master/src/main/iidxhook4-cn/avs-boot.c#L81

What specific steps should be taken? For example, making it configurable via conf file? The reason for hardcoding the log level was to follow the existing implementation. https://github.com/djhackersdev/bemanitools/blob/master/src/main/jbhook1/avs-boot.c#L118 https://github.com/djhackersdev/bemanitools/blob/master/src/main/iidxhook4-cn/avs-boot.c#L81
gar6a9e commented 2024-03-20 23:45:16 +03:00 (Migrated from github.com)

The testing has been completed.

The testing has been completed.
icex2 (Migrated from github.com) reviewed 2024-03-25 16:14:26 +03:00
@@ -0,0 +95,4 @@
{
log_info("Called my_avs_boot");
avs_boot_replace_property_uint32(config, "log/level", 4);
icex2 (Migrated from github.com) commented 2024-03-25 16:14:25 +03:00

I see, that's in general an issue that the current setup with logging does not allow for centralized log level management on the older games. I believe this requires more effort to be re-worked properly. Thus, I consider your approach valid and reasonable. Thanks for clarifying.

I see, that's in general an issue that the current setup with logging does not allow for centralized log level management on the older games. I believe this requires more effort to be re-worked properly. Thus, I consider your approach valid and reasonable. Thanks for clarifying.
icex2 commented 2024-03-25 16:16:37 +03:00 (Migrated from github.com)

Thanks for your efforts and testing your changes across all versions. Changes overall are lgtm and fine to be merged.

Thanks for your efforts and testing your changes across all versions. Changes overall are lgtm and fine to be merged.
Sign in to join this conversation.