Fix cursor skipping, and find out why things about tiles get weird

[ahicks92]

TLDR: deprecate get_selected_ent, rename it to get_selected_ent_deprecated for lack of something better, if I am reviewing a PR and see it I will hard veto with prejudice. Use iterate_selected_ents for iteration functionality that filters out the player. Reorganize cursor_skip_iteration to be slightly less complicated. See below for the long version.

The cursor decided to be amusing and started always skipping by exactly one if starting on a tile with an entity. I don't actually know when this broke. It happened somewhere between cursor rolling (before the rename) and the tip of next-update, and git blame hates us because of the reformatting commit. The source of the bug goes back basically forever, and a dose of luck caused us not to hit it.

That lead to a rabbit hole. get_selected_ent was not a getter, it was also silently trying to manage a cache. The idea was to get entities on a tile while ignoring the current player, but it has a number of surprising edge cases for 10 lines and even a discussion on Discord between all 3 of us couldn't get to the bottom of it. We know that the mutation piece is broken because commenting out the table.remove is what "fixed" the issue (this pr being the real fix).

So, we kill it with prejudice. Instead, we can implement a proper Lua iterator that isn't stateful, which is O(N) as opposed to the O(N^2) which would be required by a function like this that wishes to ignore players, , and then we can make things less error-prone by having that iterator skip invalid entities (in general you want to handle that low down; a higher level check does no harm,but they are easy to miss).

Put another way:

for ent in iterate_selected_ents(pindex) do
-- Ent is valid for the duration of this function/whatever,
-- long as you don't return to Factorio,
-- and not the player who called it.
end

Now works, and does what the old functions were actually doing, except you can do it repeatedly without refreshes.

Lastly, we refactor cursor_skip_iteration slightly by pulling common code out to a closure compute_current. In addition to the problems above, it was also having issues with start vs current mismatching; I did not debug that because I'd fixed it at that point. That causes an immediate stop. By using compute_current for both start and current, we guarantee they run the exact same check and start equal. I will not swear they can be equal but on the wrong entity, but I do believe this to be impossible. That function deserves more cleanup however.

Because we are gearing up for a release, we cannot address all of the cases, and in fact still mix cursor skipping code with it. To deal with that, we fold refreshing the tile cache into compute_current. But the following two concerns in general remain:

• Refreshing the tile hits Factorio's surface API multiple times for small searches. That's probably inefficient.
• I'd be willing to bet some of the scanner crashes are related in some way. Pretty much anything working with tiles hits one of these, even reading. We really need to get the rest of the codebase off it. At least you can't forget it's special anymore.

[LevFendi]

I can probably write a real get function that does not do the popping part. For almost every case we actually prefer reading instead of popping.

[ahicks92]

Yes, you return fn, nil, nil for iterators that don't use the state or need an initial value for var. It may be worth reading section 3.3.5 of the Lua manual. I can't seem to get a direct link to the section, seems they maybe don't support that, sorry. Technically speaking it should work by just returning fn, but this is weird and I think that it's better to return 3 values from a 3-value-returning function, even if Lua allows you to not.

After that it's simple enough: fn returns not-nil to continue iteration, otherwise nil. Turns out that after many readings your code never sees the first value. It's kinda silly, makes no sense, and took me a while to figure out. These are much easier to use than to write.

[ahicks92]

Should have clicked reply on the last one, sorry. But this double post is to say that I will update the description from today's findings via Discord this evening and then one of us should merge. It would be good to clarify what/when I'm allowed to just click that button rather than going through @LevFendi to click it, but for now I'm assuming it'll be him.

[ahicks92]

I renamed the rename. Also updated the comment. I'm going to be playing this branch but since next-update is more broken than this could possibly make it, it's ready for merge in my opinion. The only risk I see is if renaming the rename or the original rename missed one or more, which is an easy enough fix.