jbhook2 and jbhook3 updates - [merged] #211

Closed icex2 closed 1 year ago

In GitLab by @HAYU on May 6, 2022, 23:15

Merges jb-window-showcursor -> master

Common

add option -s (Show mouse cursor on game window)
remove option -f (Run the game in a framed window)

jbhook3

windowed mode actually makes it work

It was inconvenient for jubeat to be fixed only on the top left corner of the screen, so I looked at the source and noticed that windowed mode works in jbhook2, but It does not work in jbhook3. The separation of -w and -f is meaningless on jubeat, so -f was removed and windowed mode now works well in jbhook3.

I am not good at using git and code, so please tell me if there is any problem with my changes.

Thank you for your contribution. I think the general idea you submitted so far is already good. However, I assume you are not very familiar with the structure of the project, yet, which is a problem for a first contribution. I will provide suggestions and guidance on relevant parts of the code to help you get to know the project a little better and increase the quality of the contribution.

I think this easy one is a great start to get more familiar with bemanitools's structure. jbhook implementations have a bunch of things in common. Code that is shared and used on more than one hook live in src/main/jbhook-util-p3io. Take a look there and you will find a gfx module (src/main/jbhook-util-p3io/gfx.c). This module already takes care of abstracting gfx related functionality. It might be a bit overkill in this case, as we are talking about a single line with ShowCursor(TRUE) here. However, since the necessity of such an abstraction is already given due to the other modules, I suggest the following:

Create a function in the gfx module called void jbhook_util_gfx_show_cursor(); which wraps the ShowCursor(TRUE); call. Line 204 can be replaced with jbhook_util_gfx_show_cursor() then. Please remove the log statement in line 203. I don't think this will help us with debugging as you should see the cursor or not depending on the setting.

Please also consider running our code formatter using make code-format to stick to our style guide.

I read your reasoning about removing this in the MR but I am having trouble understanding it:

The separation of -w and -f is meaningless on jubeat, so -f was removed and windowed mode now works well in jbhook3.

Does that mean enabling framed with -f just breaks window mode and cannot work as on other games?

Can this be re-used with the other jbhook implementations? Considering this is a rather large chunk of code, I suggest that this should at least live in its own gfx module in jbhook3 inside a function called void jbhook3_gfx_set_windowed().

Another function might be void jbhook3_gfx_show_cursor() which enables showing the cursor.

Let me know if anything is unclear or you need further support.

In GitLab by @HAYU on May 8, 2022, 03:21

Commented on src/main/jbhook2/dllmain.c line 203

Thank you for your kind guidance.

The log is inserted because there is a difference in the actual behavior depending on the location of the if statement, but I agree that it is not necessary at the moment. The Show Cursor option is added to both jbhook2 and 3, but only jbhook2 uses gfx.c. Where should we create a function?

In GitLab by @HAYU on May 8, 2022, 03:45

Commented on src/main/jbhook2/options.c line 37

I think the -w, -f option does not work because jubeat uses OpenGL instead of D3D. So I was also unfamiliar with the way jbhook2 implements the windowed mode.

jubeat is basically a frameless windowed mode. Unlike other games, it does not become a full screen as the resolution changes when the game boot. For the same reason, if you use the -w option for jbhook2, you will get a window frame, and the -f option is meaningless.

In GitLab by @HAYU on May 8, 2022, 03:53

Commented on src/main/jbhook3/dllmain.c line 129

The mwindow_create hook can use only below copious, and the GFWin32MainWindowRun hook can use only above saucer. And jbhook2 has all the code related to the windowed option in dllmain even though gfx is already separated. I thought there would be a reason for this and used the same method of jbhook2 as much as possible.

In GitLab by @HAYU on May 8, 2022, 03:56

added 4 commits

7167b42e...e4f7c4e6 - 3 commits from branch djhackers:master
e6e0000a - Update src/main/jbhook3/dllmain.c, src/main/jbhook3/options.c,...

Compare with previous version

In GitLab by @HAYU on May 8, 2022, 04:24

added 1 commit

b41da516 - remove log for show cursor

Compare with previous version

Thank you for clarifying this. I wasn't aware of that detail and I think it makes sense to remove it in this case, as it doesn't add any value and rather confuses users.

We can always add it back, if, for some reason, this becomes viable again.

Ah, so this is another case of we need two different implementations for the "p3io era" type of games (up to saucer supported by up to jbhook2) and everything after that (jbhook3). In that case, @mon already started separating various things accordingly and I suggest following the same structure if that makes sense here as well:

Have a look at src/main/jbhook-util-p3io/ which contains auxiliary/utility code used on jbhook1 and jbhook2. I suggest to move mwindow_create hook to the gfx.c/.h module there by creating a new function void jbhook_util_gfx_set_window_mode(). Apply the same idea to "hide" the ShowCursor() call by moving it behind a function called void jbhook_util_gfx_show_mouse_cursor().
To keep things simple for now, for the jbhook3 library, I suggest to create a separate gfx.c/.h module in src/main/jbhook3 and move the code behind a function called void jbhook3_gfx_show_cursor() and void jbhook3_gfx_set_window_mode() accordingly. You probably want a void jbhook3_gfx_init() function as well which sets up the hook table by calling hook_table_apply(...). Follow with calling the new functions in jbhook3 which should streamline the code, improve readability and structure.

I hope I wasn't too brief with this, as I still would like you to take the step and actually do the execution of applying the change. I think this is a valuable exercise to get an understanding of the general structure BT5 usually uses.

In GitLab by @HAYU on May 10, 2022, 07:20

Commented on src/main/jbhook3/dllmain.c line 129

OK. jbhook1 also uses mwindow_create hook, but it is very different from jbhook2. and I don't have H44/I44 dump data, so I can't test jbhook1. I'll practice calmly with jbhook3

In GitLab by @HAYU on May 10, 2022, 11:25

Commented on src/main/jbhook3/dllmain.c line 129

changed this line in version 4 of the diff

In GitLab by @HAYU on May 10, 2022, 11:25

added 1 commit

84efbb23 - add 'gfx.c/h' for jbhook3

Compare with previous version

In GitLab by @HAYU on May 10, 2022, 11:29

added 1 commit

4077eaa6 - modify indentation

Compare with previous version

In GitLab by @HAYU on May 10, 2022, 11:58

Commented on src/main/jbhook3/dllmain.c line 129

I added gfx.c/h to jbhook3. preferably I didn't want gfx.c to call for options. but ShowCursor (TRUE); must be in my_GFWin32MainWindowRun to affect the game window. There is a way to hook USER32.dll, but I think it is inefficient.

+) Now that I see it, I don't need to the log twice. lol please review other first.

Nice, we are getting there. I think I see your point with you requiring options here. The solution we applied throughout other modules in BT5 is the following:

static bool jbhook3_gfx_windowed;
static bool jbhook3_gfx_show_cursor:

void jbhook3_gfx_init(void)
{
    hook_table_apply(
        NULL, "gftools.dll", init_hook_syms, lengthof(init_hook_syms));

    jbhook3_gfx_windowed = false;
    jbhook3_gfx_show_cursor = false;

    log_info("Inserted gfx hooks");
}

Then have a separate function that only sets static variables for the module

void jbhook3_gfx_set_windowed(bool windowed)
{
    jbhook3_gfx_windowed = windowed;   
}

void jbhook3_gfx_show_cursor(void)
{
    jbhook3_gfx_show_cursor = true;
}

Then, my_GFWin32MainWindowRun uses those static variables within the module and you don't need to leak the options struct into the gfx module which creates a cleaner interface for the whole module.

Don't forget to call jbhook3_gfx_set_windowed and jbhook3_gfx_show_cursor in jbhook3/dllmain.c with the parameters from the options struct there.

In GitLab by @HAYU on May 12, 2022, 06:11

added 1 commit

8b36906e - update gfx to receive options

Compare with previous version

In GitLab by @HAYU on May 12, 2022, 06:16

Commented on src/main/jbhook3/gfx.c line 67

Oh, It was what I wanted. I thought about whether I could deliver the option effectively, but I thought that the concept was not established and the amount of code would increase.

I referred to it because bsthook and ddrhook seem to be using the method. Tell me if there's anything that's not efficient.

In GitLab by @HAYU on May 12, 2022, 06:19

added 1 commit

eab4d936 - update gfx to receive options

Compare with previous version

In GitLab by @HAYU on May 12, 2022, 06:47

There's a stupid add history left. I am ashamed, but I hope that the changes will not be affected

Oh, It was what I wanted. I thought about whether I could deliver the option effectively, but I thought that the concept was not established and the amount of code would increase.

I mean, there is nothing wrong with that regarding "it works". However, this approach hides the fact that the module is being externally configured. It also creates tight coupling that you cannot configure the module without the external configuration.

Not really a matter of efficiency in this case but rather a matter of clean separation of concerns and clear interfaces of modules.

Something that doesn't matter here but I highly suggest to have the jbhook3_gfx_init() call before setting any options. We have several modules that depend on such order as many "module init calls" set-up the module by initializing data structures, allocating memory etc. This should always be done before calling any other functions of the module.

Even it doesn't apply here, the general practice is as explained above which creates, at least to me, above expectation. I suggest following it if there is no reason to not do so.

Therefore, I suggest:

    jbhook3_gfx_init();

    if (options.windowed) {
        jbhook3_gfx_set_windowed();
    }

    if (options.show_cursor) {
        jbhook3_gfx_set_show_cursor();
    }

Don't worry about that. Your contribution is well focused that I can just squash the history before merging into a single commit.

In GitLab by @HAYU on May 13, 2022, 04:26

added 1 commit

58dbce71 - change call order

Compare with previous version

In GitLab by @HAYU on May 13, 2022, 04:28

Commented on src/main/jbhook3/dllmain.c line 54

Thanks to you, I learned a lot of things that I didn't know

In GitLab by @HAYU on May 13, 2022, 10:44

added 8 commits

58dbce71...fb912e2c - 2 commits from branch djhackers:master
d60e4c38 - Update src/main/jbhook3/dllmain.c, src/main/jbhook3/options.c,...
fdea434a - remove log for show cursor
c72b495a - add 'gfx.c/h' for jbhook3
931d2e1e - modify indentation
8736d354 - update gfx to receive options
56b5e8bd - change call order

Compare with previous version

Happy to hear that. Thanks for sticking with me until the end. Really appreciate the effort which increased the quality of the contribution.

resolved all threads

djhackersdev / bemanitools

jbhook2 and jbhook3 updates - [merged] #211