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

Fix vjoy activation condition handling #407

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

edwardwbarber
Copy link

This has come up in the discord a couple of times (and mentioned here as well #361). Decided to take a look myself to fix the issue. This fixed the problem on my end. Happy to provide a compiled win x64 binary if that is of use too.

Here are the changes from the commit itself:

Previously all vjoy activation conditions would be reset to the default value when the UI was refreshed. This was due to several lines in VJoyConditionWidget._modify_vjoy which were intended to reset to a default value if the VJoy input was modified. Unfortunately, _modify_vjoy is called every time the UI is reset, thus resetting the condition trigger in addition to the intended
effect of updating the selected VJoy input.

To avoid the reset, we simply removed the lines that change the activation condition within _modify_vjoy.

@WhiteMagic
Copy link
Owner

While the fix will solve one issue I'm unclear whether it will cause another one. The true underlying issue is that the modify function is used both for initialization, where the defaulting of variable states is important, and updating whenever a vJoy value changes, where this defaulting is not intended.

Looking a bit more at the code, outright removal of the input type based checks will cause bugs. The reason for that code is to enforce the comparison value to have a valid value as it is possible to change the vJoy input type from button to axis or similar. Not changing the input type of the comparison value will break other UI aspects and likely cause crashes. Thus, the actual fix would need to check whether the comparison value needs to be defaulted due to an input type change on the vJoy side.

Also, the _modify_vjoy method isn't technically called upon a UI reset, it causes one, but rather when the vJoy selection changes, which can also happen during a UI reset as there is some amount of cyclical stuff in the UI aspect going on.

Lastly, it is not likely that any of these fixes will be merged as R14 does not reduce any of the UI, and most if not all of the action logic has to be redone as well. While fixing some of the remaining common bugs would be nice, integrating, testing, etc. of those would eat whatever little time I have to work on R14 which is why I'm unlikely to do any of that.

@edwardwbarber edwardwbarber force-pushed the BUGFIX-vjoy-condition branch from ba96c09 to 7a6e28a Compare July 12, 2022 03:12
    Check if current comparison condition is valid for the type of
    VJoy condition under `_modify_vjoy`. Only reset comparison
    condition if it is not valid for the current input type.

    Previously comparison condition would reset with any call to
    `_modify_vjoy`.
@edwardwbarber edwardwbarber force-pushed the BUGFIX-vjoy-condition branch from 7a6e28a to d52c2d6 Compare July 12, 2022 04:39
@edwardwbarber
Copy link
Author

You are right. I did not consider the comparison value state if the input type is changed but not the comparison selection within the UI. Reverted my change and added some logic such that the comparison value is updated only if the current comparison value does not match an expected one for the type. A little clunky, but it seems to work.

I understand that you don't have the time to integrate these changes yourself, but I'd still hope this fix is useful to folks in the community that are willing to build their own versions (few of us though there may be!). Thanks for your efforts all the same.

@WhiteMagic
Copy link
Owner

This looks good, there are probably some ways to make it less clunky but really it would need proper enums for the various conditions and inputs, things that don't exist in that code base, at least not for the conditions. So this is a good middle ground.

Yeah, that is why I comment and leave the pull requests open so that people actually can see them for their own use. Also helps me to remember things that I need to keep in mind when implementing those functions. This kind of bug and annoyance with condition types for example doesn't exist anymore in R14 conditions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants