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

Zoom In/Out key binding fixes #2767

Merged
merged 10 commits into from
Oct 21, 2022
Merged

Conversation

darkdrifter
Copy link
Contributor

@darkdrifter darkdrifter commented Sep 25, 2022

Resolves #524

Hello,

This is my first contribution to any open source project, tried to follow the guidelines and coding style but I may have missed some things. Don't hesitate to tell me if I did.

Threes fixes are included in this pull request:

  • Fix for the inverted zoom in/out bindings. For some reason, CControllerState RightShoulder2 controls Zoom Out, while the CControllerState LeftShoulder2 controls the Zoom In, I don't know if it's the game behavior on PS2.
  • Fix for the Zoom In/Out behavior being also controlled by the Weapon Previous/Next key binds. This behavior isn't present in the base game so it shouldn't be in MTA.
  • Thanks to the fix just above, we can remove a hack that disconnected Zoom In/Out behavior using mouse wheel to try and fix zoom being applied twice. I think it was actually due to the fact that Weapon Previous/Next was applied first (if bound on the scroll wheel) and them Zoom In/Out gets applied and adds another zoom on top of it. With that hack removed, we can now bind mouse scroll wheel for the zoom. On master if you try to bind the scroll wheel only on the zoom in/out, you'll observe that the zoom isn't working. (moved into Fix weapon zoom behavior #2769)

Don't mind the first two commits, it was a first try to fix the issue that I reverted after I dug up further and found what I think is a better fix.

@darkdrifter darkdrifter changed the title Weapon aim key binding fixes (Fixes #524) Weapon aim key binding fixes Sep 25, 2022
@lopezloo lopezloo added the bug Something isn't working label Sep 26, 2022
@darkdrifter darkdrifter changed the title Weapon aim key binding fixes Zoom In/Out key binding fixes Sep 26, 2022
@lopezloo
Copy link
Member

I think it was actually due to the fact that Weapon Previous/Next was applied first (if bound on the scroll wheel) and them Zoom In/Out gets applied and adds another zoom on top of it.

I haven't tested your PR yet but I have unbinded all keys (via unbind all command) except aim_weapon and I'm still being able to zoom in/out sniper using scroll wheel. (tested on v1.5.9-release-21326)

@darkdrifter
Copy link
Contributor Author

darkdrifter commented Sep 26, 2022

I tried with unbind all on v1.5.9-release-21331 and I can't zoom using mouse scroll until I bind mouse scrolling to Weapon Next and Weapon Previous
I'm testing in Map Editor in "Full Test" BTW
EDIT: Tried in a online server, same behavior.

@lopezloo
Copy link
Member

lopezloo commented Sep 26, 2022

Looks like it actually depends on single player control settings. I have unbinded it in single player and it doesn't work anymore. It shouldn't work like that.

single_player

@darkdrifter
Copy link
Contributor Author

darkdrifter commented Sep 26, 2022

Yeah indeed when I set the mouse scrolling in single player I get a smooth zoom in/out even with everything unbound in MTA. And if I bind mouse scrolling in MTA I get a jerky "double zoom", probably the thing that the hack tried to fix.
I'll tried on my build with this PR, I don't have the said "double zoom" if I bind the mouse scroll on solo and MTA, though I still have mouse scrolling when I unbind all if I have it set in solo. So it's an improvement but there is still something weird going on.

Fix weapon zoom in/out getting affected by single player key bindings.

We now hook Direct Input scroll wheel data as the game seems to use this value rather than the windows messaging system to handle zoom.
@darkdrifter
Copy link
Contributor Author

I found the issue with the scrolling that caused it to be affected by single player. It seems that GTA SA uses Direct Input to get the scroll wheel value when using mouse scroll wheel on zoom. It's probably to have smoother zoom, as now that I have hooked the zoom value on direct input, the single player no longer affects MTA but the scrolling is a bit harsh.
As I described in the comment where I placed the hook, a better option could be to instead give this data only when we want it (when zoom is bound to scroll wheel in MTA) to retain the proper smooth zoom, but it would require also successfully forcing the game to use this data even when it is not bound in single player, but I haven't looked closely at this option yet.

@lopezloo
Copy link
Member

lopezloo commented Sep 27, 2022

That mouse wheel zoom logic is defined in CCam::Process_M16_1stPerson(). It calls CControllerConfigManager::GetMouseButtonAssociatedWithAction(&ControlsManager, PED_SNIPER_ZOOM_IN). The question is - why does it seems to be returning something if it's not defined in MTA binds? We need to investigate it more since it would be nice to have smooth zoom.

@lopezloo
Copy link
Member

lopezloo commented Sep 28, 2022

I suggest to remove that mouse wheel zoom fix and create seperate PR for that instead.

Related issues:

Jump bind also triggers zoom in (it doesn't happen in single player).

@bum8hj
Copy link
Contributor

bum8hj commented Sep 28, 2022

Jump bind also triggers zoom in (it doesn't happen in single player).

This is slightly off-topic, but I believe jumping also forcefully re-enables aim_weapon when it's disabled with toggleControl.

@darkdrifter
Copy link
Contributor Author

Hello, I reverted the last commit that I will move into a WIP PR that will look into the whole zoom behavior issues (to the best of my abilities).
I don't know how to quickly reproduce issue #2275 or #2570 to see if it's still present with this PR, but it would be nice to know if it fixes them indirectly.
@lopezloo we stop this PR with only this fix then and integrate it with its improvements even if some flaws are still present ?

@lopezloo
Copy link
Member

lopezloo commented Oct 1, 2022

@lopezloo we stop this PR with only this fix then and integrate it with its improvements even if some flaws are still present ?

I just would like to merge fix for inverted controls first. Please restore that zoom check which MTA used to have (and then remove this condition in your other PR).

This is slightly off-topic, but I believe jumping also forcefully re-enables aim_weapon when it's disabled with toggleControl.

Unable to reproduce that.

@darkdrifter
Copy link
Contributor Author

Please restore that zoom check which MTA used to have (and then remove this condition in your other PR).

Done ;)

Copy link
Member

@lopezloo lopezloo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you.

I have slighty edited first post.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zoom in/out binds are inverted
3 participants