Refactorio / RedMew

The RedMew scenario code for Factorio.
https://redmew.com
GNU General Public License v3.0
176 stars 80 forks source link

Thoughts on antigrief.lua #186

Open grilledham opened 6 years ago

grilledham commented 6 years ago

I like this feature, but I have a few concerns about how it works and some ideas for things to change:

Valansch commented 6 years ago

I agree. I had this discussion internally and decided to use the original persons last user. Both have their pros and cons. I don't think that griefing accusations would be a big issue. But it would make it more consistant and intuitive if the admins name is on the entity. (If you cancel a deconstruction your name will be the last user). Also it will make the code easier.

I don't think this will ever actually happen. All entities will be unpowered. Even if some odd solar panels or the odd steam engine with a coal buffer may run. I think thats gonna be the exception. Therefore UPS won't be an issue.

Don't block players

I don't think thats a problem. But yes we can make them go into spectate mode.

Another pro would be: Less script updates. I think we should keep script updates down and the global table down wherever we can. I don't think if it actually matters or if its just my preference.

I agree with your cons. I think they outweigh the pros.

We might want to consider making one antigrief surface per real surface

I actually consideres this, most parts of the code should support multiple surfaces. Some parts don't that would be rewritten. I just didn't do it to keep things simple as a proof of concept. I have nothing against multiple surface support. Although it may be a low priority.

I agree. I just found for custom commands to own it more object oriented. Maybe we can find a middle way? Maybe we call a function from control.lua that disables/enables antigrief?

I would like to make a camera view into the antigrief surface

That would be helpful. But mostly for actually finding the griefer. I don't know if we should allow players to view the ag_surface(s). Mostly because i don't want the griefers to know the tools we have. Makes them griefe more creatively. I also don't want to get to far away from vanilla as to not scare people away. But thats just my opinion.

PS:

Current;y map_layout needs to run after custom commands, because some map modules add commands.

We should fix that. Maybe put the code that hijacks commands.add_command into utils?

grilledham commented 6 years ago

Ghost entities / Performance

I think we need more data to make a meaningful decision here. I ran an experiment where I load a save, set game speed to 100, go into map view and zoom out fully. Then record what I perceive to be the average stable ups I get. Then delete the antigrief surface and record the new ups. For the final save for the beach map I went from 228 to 330, for the factory map a save from this morning I go from 530 to 620. It would probably be good if you performed similar tests so we can compare results.

It's also worth nothing that I have no evidence ghost entities would perform better, I just assumed they would. It can be surprising which entities cause problems and which don't. I ran another experiment where I generated a void map with nothing but small power poles every 8th tile to prevent them from connecting. With 1600 poles It took 6ms to process the Electric network. Connect all the poles and it takes less than 0.1ms.

Disable in control / custom commands

I'm not sure what to do here we have opposite opinions on what to do with custom_commands.lua. But for now we could solve the problem with an init function in antigrief.lua that registers the events, and just call it in control.

I agree that we should move the commands.add_command function out of custom_command. Maybe we make a init.lua for stuff that has to run before any other module. But I can't think of any other code that would go in there yet.

Camera

I agree that letting the griefers know about the tools we use doesn't seem wise. But I like trusting people, so I would vote for letting regulars view the camera. I think if we give regulars more powers to find griefers that only helps us. Also in my experience it's really easy to tell when someone is a griefer or not normally within less than 30 minutes of play time, so I do trust our regulars.

If we do allow regulars to use the camera, it might be a good idea to raise the on_player_promoted event when a player is promoted to regular, or we add new events something like

defines.events.on_regular_promoted = script.generate_event_name()
defines.events.on_regular_demoted = script.generate_event_name()

So I can add and remove the camera for the player. If we add new events that code would have to run before any other module

Different surfaces

I agree that it's a low priority, I don't have any current plans for multiple surface maps.