DerJantob / TSW2_Controller

Control TSW2 with a joystick or other controllers
25 stars 4 forks source link

Fix for English master controller screen reading #35

Closed asdf1280 closed 2 years ago

asdf1280 commented 2 years ago

In some trains and English version of the game, the text is displayed like this:

Throttle / Brake nn% Power

Throttle / Brake nn% Brake

Thus, it's not possible to detect whether the train is in brake mode or throttle mode only with one word like 'Brake', as the word 'Brake' is always included in the header.

However, if this software could detect text indicators like '% Brake' it would be possible to tell current state of master controller lever. This is a fix for this issue.

asdf1280 commented 2 years ago

Hello. I'd like to inform you that this PR is work in progress. I will try to make an overall fix for master controller detection issues. I'm testing various methods now. Please don't accept this PR. It's pretty unstable.

asdf1280 commented 2 years ago

In fact, I have found codes in 'bgw_readScreen_DoWork' not being run at all in master controller mode, where 'brakeConfig' becomes null. The call of 'brakeConfig[0]' will cause exception and code past that line won't run at all. This part is gonna need a major overhaul.

asdf1280 commented 2 years ago

Hello. Finally, I think this PR is stable enough and running as expected. Please have a test and tell me your opinions.

DerJantob commented 2 years ago

I think I don't quite understand your problem. I have tested your example and for me, it worked fine.

I have added "Throttle / Brake" to the text indicator (Throttle / Master Controller) and have "Power" and "Brake" for the Master Controller areas. Now if the program gets the information "Throttle / Brake 12% Power" or "Throttle / Brake 12% Brake" the program detects "Throttle / Brake" and removes it in line 1843 1843: result = result.Remove(0, result.IndexOf(config[0]) + config[0].Length).Replace("\n", ""); 1844: result = result.Trim(); so you are left with "12% Power" or "12% Brake". After that, it detects the Throttle area or Brake area without any problems. Could you maybe explain to me again exactly what the problem is?

However, the fix for the exception caused by the config being null looks good.

By the way, respect to you that you are able to understand my code :)

Kind regards Jannik

asdf1280 commented 2 years ago

Hello. I think it's quite hard for me to explain code detection further, as I had no idea either. For me it just didn't detect well and after trying a few methods, that worked. If the current one's working for you perfectly, then there might have been a problem with my configuration so you can ignore that part. So I'm gonna remove that modification I made. And about null handling, I will create them in a new repo and PR. So you can accept them.

asdf1280 commented 2 years ago

I believe the critical part was null handling, not something related to text indicators.

DerJantob commented 2 years ago

I think you don't have to remove it. I mean, if it works better for you (and maybe for others) it is an improvement. I haven't tested much, but it looks like that the changes should not influence current settings, but just give more options with the indicators.

asdf1280 commented 2 years ago

About text indicators, I don’t think it would work stable in this state. I can’t guarantee stability of this change. Rather than adopting this detection method, I would suggest text indicator regex.

DerJantob commented 2 years ago

Regex looks very interesting. Since I'm thinking about reworking the whole system, that would be something I will have to try out.

asdf1280 commented 2 years ago

Also, can I ask you what the function 'ContainsWord' does? I think it does something identical to 'string.Contains' method and can be changed like this:

        public bool ContainsWord(string stringToCheck, string word)
        {
            return word != null && stringToCheck.Contains(word);
        }

or, if you are trying to only match the whole word, use this. This also includes regex support and for me it worked correctly after changing like this.

        public bool ContainsWord(string stringToCheck, string word)
        {
            return word != null && Regex.IsMatch(stringToCheck, $@"\b{word}\b");
        }

Regex support, but without forcing end of word:

        public bool ContainsWord(string stringToCheck, string word)
        {
            return word != null && Regex.IsMatch(stringToCheck, $@"\b{word}\b");
        }

Personally, I found the third option working best. Which one would you prefer?

DerJantob commented 2 years ago

The function "ContainsWord" is checking if a whole word is inside of a string. So it is true when you are searching for "good" inside of "This is a good test" but false when you are searching for "good" inside of "This is agood test".

In your reply your two bottom examples are the same, but looking in your commits i found this:

return Regex.IsMatch(stringToCheck, matchWholeWord ? $@"\b{word}\b" : word);

I do not understand what $@"\b{word}\b" does, but it seems to work as intended. If I understand correctly, "matchWholeWord = true" causes it to not ignore upper and lower case, right?

asdf1280 commented 2 years ago

\b means ‘to match the end/beginning of a word’. Can be a bit hard to understand, but it does surely what you have wanted ContainsWord function to do. And I found the way to make it case insensitive here : https://www.dotnetperls.com/regexoptions-ignorecase

asdf1280 commented 2 years ago

Let me add a reminder here:

result = result.Remove(0, result.IndexOf(config[0]) + config[0].Length).Replace("\n", "");
                result = result.Trim();
DerJantob commented 2 years ago

I don't quite understand what you mean with the second point yet. How would the lines of code you mentioned affect the regex check?

asdf1280 commented 2 years ago

Hello. The thing you wondered about is in 'Full regex support for text indicator detection'. The work related to regex seems to be ready now. Please do some test for it.

Also, please be aware that regex text indicators might break 'korrigiereReadScreenText' function. I'd suggest if regular expressions(like backslash) are detected in text indicator - config[0] , such function should not be run.

asdf1280 commented 2 years ago

To test regex text indicators, you can try indicators like this(English version of the game):

Me?o?a?ster Controller - Master Controller | Measter Controller | Meoaster Controller | Moaster Controller | Moster Controller (To fix OCR issues)

Throttle\s?/\s?Brake - (To fix OCR space detection)

DerJantob commented 2 years ago

But wouldn't I have to add the whole thing manually for "Me?o?a?ster Controller"? With "korrigiereReadScreenText" I automatically detect how similar the scanned indicator is to the indicators the user has entered (not how similar looking the letters are but how many are different). OCR sometimes detects weird stuff ("l" as "/" for example). Wouldn't that be a little hard to do with regex?

asdf1280 commented 2 years ago

I agree with your opinion. For people who know regex very well and are willing to calibrate it one by one, regex can greatly improve detection if the regex is supported for sure as I have tested. However, for normal users, it would worsen the detection. So, how about adding an option so that people can choose whether or not they want to use regex or plain text detection?

I think settings can be made like the following image: 제목 없음-1

And if this option is added, the software would work like this: If regex is enabled, use the current version of PR but with removed 'korrigiereReadScreenText'. If not, we can call regex related functions with Regex.Escape(pattern) method that C# provides and keep using 'korrigiereReadScreenText'.

asdf1280 commented 2 years ago

However, I don't know how to add a setting to the software. So, could you create a new branch from v1.1.2 and accept my PR to that branch, and then add the setting element? I can then finish remaining logic parts.

DerJantob commented 2 years ago

I don't know if it's worth it for you to put so much work into it now, since I'm currently working on changing a lot. Since you have more than 2 controllers in most trains, I plan to make it possible to control all of them. I imagine it like this: grafik

1: Here you can select the different controllers (like throttle, brake or the indirect brake) 2: Add the selected controller to a train 3: Edit controllers (this leads to the next image) grafik

Here you can edit the controller, so you can define what keys correspond to this controller and what indicators are used for it. It might be a bit hard to understand, because the words are still in German, but maybe you understand what I mean. If you want, you can take a look at it ("NewControls" branch), but it is very unstable currently and have added almost no comments yet. Feel free to tell me what you think about it, but you do not have to.

However, I have created a branch for your PR. It is called "v1.2.1_PR_asdf1280"