search

destruc7i0n / shulker

A Discord to Vanilla Minecraft Chat Gateway
ISC License
135 stars 47 forks source link

Fix death message false positives #71

Closed MageLuingil closed 2 years ago

MageLuingil commented 3 years ago

This fixes issue #70 by replacing the death keyword config option with a death message regex config option.

destruc7i0n commented 3 years ago

Nice work! Thanks for the report and fix.

Seeing as this is removing an option for the config, do you think you could add a message somewhere (maybe loadConfig in Shulker.ts) alerting those who may have the old config line that they need to update their config to correctly support this? Not sure myself about how many actively upgrade the bot, but would help those who do.

MageLuingil commented 3 years ago

That sounds like a really good idea. I presume you're thinking just a warning to the console? I haven't tested with that option still present (and I'm still new to typescript) so I don't know if unexpected config options will throw errors.

In any case, I'll look into adding that when I have some time later.

destruc7i0n commented 3 years ago

Yeah just a warning for now. I should add some better config handling, to automatically add defaults for changes like these...

MageLuingil commented 3 years ago

Okay, I added a configurable array for config option deprecation notices. Not sure if my way is the "best" way to do it in typescript, but there's at least a deprecation notice now, pointing people to the README.

Further feedback is of course welcome!

MageLuingil commented 3 years ago

On a side note, my death message regex is based off of information from the Minecraft wiki (which has on occasion been wrong). Comparing my list to yours, it looks like I'm missing the keywords "finished", "speared", and "removed". I see "finished" in the list of removed death messages on the wiki (it appears it should have been handled by other keywords anyway), but I don't see any references to the other two.

Do you happen to know what the death messages for "speared" and "removed" were, and if I need to add them to my regex?

destruc7i0n commented 3 years ago

The best place to reference the death messages would be the language files. I attached a trimmed version of the latest en_us one for reference.

I do not see them either, not sure exactly where they came from.

@TheZackCodec had made the original PR for that, do you happen to know what those are form?

en_us.txt

MageLuingil commented 3 years ago

Thanks for pointing out the language files to me! I don't know why I didn't think to use those from the start.

So, I may have slightly overkilled this, but I compiled a list of all death messages from the EN language files for all versions 1.12 - 1.16.5. The only message I was missing previously was an old typo in 1.12 for "[player] discovered floor was lava" (missing the "the").

I honestly don't know if this project works correctly with log files from 1.12, but I'll probably add support for that death message to my regex anyway. As for those other two messages, if they were ever in the game, it was probably only in snapshots.

Edit: I can go back farther than 1.12 btw, just wasn't sure when the log file messages were changed last.

destruc7i0n commented 3 years ago

The project came out in 2016 when 1.8.9 had released. I think that 1.12 is a good lower bound, however.

destruc7i0n commented 3 years ago

@MageLuingil Hey! Just wanted to check if you had a chance to update your regex as you mentioned above. (GitHub removes from homepage after 14 days so just wanted to keep this fresh on the page)

MageLuingil commented 3 years ago

Hey @destruc7i0n! Sorry for the prolonged radio silence - I've been pretty busy IRL for the last two weeks. I do have that regex update, I just never got around to pushing it to the repo, sorry about that.

I was toying with the idea of combing through the death messages from 1.8 (or even 1.7, since that's one of the major modded versions) on up, but that was before life got busy.... Once I get some breathing room, I'll push the current fix, and then we might consider those additional messages later if you're interested (it really isn't that hard with a diff tool, just need to take some time for it).

MageLuingil commented 3 years ago

Sorry for the delay; I added that missed death message.

I'm going to work on the death messages from 1.7-1.12 as well, but it may take a little longer.

destruc7i0n commented 2 years ago

I've gone ahead and merged this, had another report which brought this to my attention again. Thanks again for the contribution! If you wish to update the death messages at another time, feel free to make another PR

MageLuingil commented 2 years ago

I made a script that compiles a list of all death messages across all release versions of the game, and checked that list against the regex currently being used. As of 1.18.1, we're catching all death messages (except for an unused one from 1.5 that later got removed). That means no more death message updates coming from me for now!

It also means future changes should be easier to find using this script (which I have here if you're interested).