Snapping seems to be entirely broken?

[Deadlock989]

I was playing with DSB in an actual game today and as far as I can tell, snapping is completely broken - whatever it is currently doing (if anything) it's not what was originally intended or is described. I took a look at the code for the first time in 18 months or so and can see that it has been heavily tweaked over time - by multiple people with conflicting preferences. It also checks for non-existent things like Nexela's made up "revived" field in on_built_entity that should never have been a thing in the first place - we put that in to make all the "bug reports" caused by Nanobots and Bluebuild go away - that kind of thing is now superseded by general changes to events in 0.18/1.0. We could also be using event filters which didn't exist 3 years ago.

There is a slim chance that some other mod I am running is breaking it, I don't think so but I haven't fully checked. If anyone else can confirm that snapping simply doesn't work, I volunteer to rip it all out and put it back to the basic scheme it had originally.

Another potential solution is that we literally just remove it. There's an argument that it causes more problems than it solves. The first ever snapping code on the mod portal was by someone called Articulating - Loader Redux uses a descendant of that original code. They called it "idiot snapping".

[shanemadden]

Huh, I just tested real quick and it's still working as desired here - there's some ways it's definitely broken when running in /editor mode when placing blueprints that I think are unfixable, but otherwise it's working alright for both the crazy mod set I was last playing on a friend's server, and with only this mod loaded. So, probably mod interaction - but, probably not worth the yak shave to find exactly which one.

https://user-images.githubusercontent.com/3057436/104488152-33096180-558b-11eb-9b2a-72c03874b9ee.mp4

Regarding the feature overall - yeah, the code grew really funky and filled with special cases and extra options. I think this is a similar case to the argument for removing https://github.com/shanemadden/factorio-deadlock-integrations from the main mod and putting it in a separate one - it makes the main mod less complicated and cleaner, and we can put a little mini-mod out for the people who actually want the behavior (or pick a different one that suits what they're looking for instead of needing to navigate the current web of options).

So, I vote for completely removing the snapping code and event handlers from this mod and volunteer the paste the existing snapping into a new independent mod for the inevitable people who want it back (can also do the less complex old version in a separate one if there's interest).

[Deadlock989]

You're right. I just tested it with DSB and nothing else and it's at least working for the basic use cases. Now I'm paranoid that it's one of my other mods that's breaking it, going to investigate it.

[Deadlock989]

So it turns to be because the IR2 Stacking patch mod is changing the item order of the loaders without using "deadlock-loader" in the order value, which is making the caching-metatable bit fail to identify them as DSB loaders. I think (from memory) you put that in to prevent the kind of greedy "all loaders are mine" behaviour that other loader mods exhibit, when people have multiple mods that provide loaders - usually we would go by entity prototype names but because we let people create tiers with any name that wouldn't work. But I kind of tripped up on it this time.

I can fix it in the patch mod easily enough. But am wondering if the entity prototype order field would be a better place to check that than the item prototype order - much less likely to be changed by crafting menu obsessives like me? Or now that loader-1x1 is separate type, just do it for any 1x1 loader regardless and let people turn the snapping option off if some other mod is double-snapping them?

With regard to pulling the whole snapping thing out as a separate mod - I'm not sure, I can see both sides of the fence. But certainly wouldn't object if you wanted to do it, I hardly use the snapping even when I do use the mod. Not sure how punters would feel about it.

[shanemadden]

Yeah, I agree with the 'just snap all 1x1 loaders' approach, and nuke the metatable cleverness, since the main reason to avoid snapping other mods' loaders was to avoid having to handle the 2x1 case when they were the same type. This solution can optionally still include yanking the logic into another mod, so that people are definitely opting in to the behavior for their loaders - but we can punt that decision for the moment.

I'll throw a quick branch together this evening with the change, unless you want the satisfaction of nuking it!

[Deadlock989]

No no, be my guest.

Just to add, another note of thanks for babysitting this wayward child for so long.

[shanemadden]

No problem at all - I just feel bad that I haven't done some of the obvious cleanup stuff like a fresh API documentation page.. one of these days!

Tagged 2.4.1 which should snap all 1x1 loaders if snapping is enabled, let me know if you see any issues.

[Deadlock989]

Works fine here. Cheers. Suggested another couple of minor tweaks on the build event in a pull request, not worth a release for that alone.