search

WhiteMagic / JoystickGremlin

A tool for configuring and managing joystick devices.
http://whitemagic.github.io/JoystickGremlin/
GNU General Public License v3.0
313 stars 46 forks source link

Fix vjoy activation condition handling #407

Open edwardwbarber opened 2 years ago

edwardwbarber commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.