Reintegrate KK with our fork of Kruise Kontrol with a remote API.

[ahicks92]

The fork is at https://github.com/factorio-access/kk-remote

See remote.lua over there for the API (with documentation!).

This also includes some fixes, since I basically rewrote it anyway. And comments. Please check the accuracy of the comments.

basically what I did was add a remote API matching our needs. It can tell you whether it's active and whatever else. The strings changed a bit, in particular the walking string is now "moved to (x, y)" literally with the parens.

Of course there's a bunch of open questions. Firstly before continuing note that this is behind the ruler change by a commit, I'll rebase before landing. The rulers are too annoying, I can't actually play with them yet. At least fixing sounds is next on my list.

Anyway, a summary:

• Remove all the references to Kruise Kontrol global state in the mod that I spotted. Maybe some left.
• Get rid of all the guesswork and call the API.
• Now it's ctrl+alt+], with changes:
  • We don't allow blueprints to be in the hand, as those seem to break it badly. For now we just flat out error and make the player decide what to do as it's definitely less than clear.
  • We turn cursor mode off. This also can freak KK out in a whole lot of ways
  • And of course we check the hand stack to see if it's a blueprint or book.
• the strings for KK are now localized (but let's discuss that elsewhere: access.long-name is annoying, can we just switch to fa before going hard in that direction?)

And now for the concerns, weirdness, and general questions.

• Didn't we do something about get_selected_ent_deprecated? Thought we were moving to a new API.
• What are we going to do about inputs? I'd rather review this early but the underlying issue is that it turns out we can't turn inputs off or disable them without breaking KK. I think we will need to come up with a dummy key and move inputs we want to disable there. I nominate ctrl+alt+shift+backspace, but let's discuss if we feel it's needed.
• A player's cursor_pos is very weird. It seems like it's including fractional components except when it isn't. Floor here seems to match right but...
• Are my changes in control.lua correct? That seems like the kind of thing which is going to have a bug.
• Can we avoid toggling remote view off? If this works with remote view on there is no need in theory but in practice...
• It looks like all our alt inputs don't work except that they do? Kinda unrelated but appareently factorio wants "CONTROLL", not "CTRL". I haven't tested. Mine didn't seem to work till I fixed it but there was a lot going wrong and it could just be human error.

Anyway I've done minimal testing. I'm thinking we don't land for like a week. It seems to work but this is a large change. To test it yourself remove the kk zip and clone the other repo directly to (case sensitive) Kruise_Kontrol_Updated.

I'll make issues or just fix them, but I noticed three things that should be followed up on:

• We don't need to always announce KK state now that we have reliable reporting. Let's consider how we want to handle that. I think exponential backoff or replacing it with a KK working sound.
• How we schedule around ticks happens in multiple ways and in addition doesn't use functions as values and one of the methods even relies on the function being global. (see schedule in control.lua). There are much simpler, better ways which can balance work without inventing some new random modulus and shoving it somewhere. easy to fold this into my pending event handling rework.
• One of the bigger cases around magic numbers instead of named constants turns out to be the walking mode. This is basically incomprehensible unless you have previously memorized the indices of the modes. We can fix this with a LuaLS enum table.
• Finally why can't we search based off just a position? Probably something @LevFendi knows but if you don't have a bounding box and search a surface off cursor_pos which I thought was fractional you don't get any entities.

[LevFendi]

Reminding myself here to check for left over references to old KK code

[LevFendi]

• let's go ahead and open a new issue for shorter localised string names
• as i said many times before, the transition from get_selected_ent_deprecated was not done for control.lua specifically

[LevFendi]

Roger that about landing later, I will need to do a fuller review anyway.

[ahicks92]

Yeah I mean I remember we literally had a whole debate back and forth and back and forth and... about when to land your changes. I just can't for the life of me remember what they are. Or how they'd work well with the deprecated function. I'm nominally fine with leaving this as it is if only because I'm around and if you don't get to tech debt I probably will. Makes me sad inside in any case.
Give me a signoff on #212 and I'll just fix the namespace. It's a particularly creative regular expression and quick manual edits.

[LevFendi]

extra KK inputs

Yes, we can give them an obscure long keybind if we cannot just pass an empty string.

our alt inputs

They work by overwriting the KK inputs in data-updates.lua, which is part of the whole build order thing.

defining custom inputs

You can find the internal names given to keys on this page.

https://wiki.factorio.com/Prototype/CustomInput

weird cursor pos

I need to repro that because i have not encountered it before

keeping remote view on

I am sure we can do that

new status reporting concept

I like the idea of a background sound like the crafting sound for conveying that KK is active. As for announcing the status, it is nice to do it when you first send a command and maybe it should also be possible on demand by pressing a keybind.

[EphDoering]

regarding search by position. I'm pretty sure it only returns entities whose position is with the radius specified. So it doesn't use the entity bounding boxes when searching.
If you're just trying to select what is at a position you could use update_selected_entity and then use the selection.

[LevFendi]

The changes in the code all seem to be good. I will test before final approval.

[LevFendi]

Why is it intentionally linked to driving? The ENTER key was selected for convenience.

[LevFendi]

Because of the linked game control, perhaps it might be that the cancel operation does not fire properly unless you are next to a vehicle.

[LevFendi]

Info: The thing about messing with walking is that telestep mode goes bonkers in multiplayer, so in this walking mode we intentionally force the player walking speed to become zero.
The new version of this file handles this matter correctly.

[LevFendi]

We will need to test if we can remove this but I prefer to.

[LevFendi]

Test session results:

• Commands given off screen behave as desired. Therefore we no longer need to force close remote view or cursor mode.
• Status reporting works the same and has the same room for improvement as before.
• Cancelling of any task other than walking does not work: You become soft locked in that action, such as chopping trees.

[ahicks92]

Ok so note that getting replies inline is a whole thing, so I'm going to put it here and then later I'm going to follow through on exploring VSCode's options for reviews which are probably better.

To copy a bunch of stuff from Discord and some:

• Will drop remote view toggling. Having remote view open a lot of the time would be kind of cool with respect to letting people get an overview when livestreaming, I think.
• Cancellation bug turned into two, one now fixed in this PR, other pending in kk-remote repo that I don't have the ability to get to at this exact second.
• I don't think update_selected_entity is appropriate yet. The mod is very bad at exposing a coherent idea of this, because the mod lets one iterate through tile entities--but that doesn't work for now in any case for other unfixable-ish reasons.
• I haven't tried empty strings for custom input disabling. I do know you must have the prototypes and they must not be disabled, or the game crashes when a mod tries to register for events. Also their official list of names includes keys that I don't think exist on 99.99% of keyboards, we can probably just use those, I will find one if needed.
• Enter is either enter vehicle (KK isn't running) or KK cancel. That's why they're related to each other. You can't be near a vehicle or it does both. It's always been that way. Changing it means either moving KK cancel to another key or rewriting the driving logic the game has for entering vehicles. I'm not opposed to just moving KK cancel though--maybe ctrl+alt+shift+]?

Note also I need to land the PR instead of you all. I'd normally use history rewrites but idk if either of you know how to deal with it and this is complex enough that people have to check out repo branches. As a consequence I'll be landing as an atomic commit with some squashing and/or history rewrites depending. Dealing with devs doing history rewrites in these cases is reasonably pedestrian and common in many projects if important devs to the project know how to deal with it. In all honestly Austin PRs are landed by Austin may be a good rule in general.

[ahicks92]

Ok, pushed readme improvements. We've also maybe made the decision to fork KK ourselves for mod portal. Also the cancellation issues should be fixed. I would like to get an approval on this so that I can move on to other coding work while we figure out the rest, which is important in this case because I will end up needing to do rebases to keep this pr in my local copy (which I want because it's almost unbelievably better UX).

We may wish to move the readme around. We can do so later but the section is now big enough that it really doesn't belong much in the keystrokes bit. Seems out of scope. We'll also have to backfill the link.

[ahicks92]

Ok, regrettably I did have to just do a history rewrite. To deal with this (back up first if you don't know what these do):

# Get a clean history
git commit ...
git fetch origin
git checkout localbranch
git reset --hard origin/localbranch

The last is destructive and will take out any uncommitted local changes.

[LevFendi]

Review complete, changes made in line comments, README, and info.json. Testing comes next...

[LevFendi]

Maybe consider renaming the interface in the code to "kruise_kontrol_remote"?

[LevFendi]

I fixed a crash for when the character is missing and I cleaned up the muting options for when the cursor mode and remote view are toggled.

[LevFendi]

I also just went ahead and formally named the magic constants that are the walking modes. This will be necessary for solving how to keep the cursor on and walking behavior work as intended.

[LevFendi]

Testing looks good and keeping cursor mode on while KK is active turns out to be more complex than expected because it messes with movement rules. More thinking and testing will be needed for that later. At the moment this PR is good to merge, except maybe for renaming the remote interface to "kruise_kontrol_remote". Great work!