Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add permissions module for DO #1391

Merged
merged 7 commits into from
Jan 14, 2024

Conversation

RedRafe
Copy link
Contributor

@RedRafe RedRafe commented Jan 8, 2024

  • Add permissions module for the DO maps
  • disable BP import for modded DO maps

Copy link
Collaborator

@grilledham grilledham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also do bob's mod as well?
I also wonder if it would make sense to do the "unmodded" danger ore presets as well? From a consistency standpoint, rather than we are just banning blueprint imports because factorio/the mod doesn't validate the blueprint properly.

@RedRafe
Copy link
Contributor Author

RedRafe commented Jan 11, 2024

@grilledham I've refactored the whole module and come to a satisfying conclusion.

The config module permissions turns ON/OFF globally the feature from enabled flag, as per any other config field; by default it's globally enabled but no sub-preset is turned on (so no restriction is applied to generic scenarios). Each DO scenario simply enables a permission preset by calling Config.permissions.presets.no_blueprints = true

For single-player-use when downloading from GitHub, they can disable/enable the no-BPs (or any other future preset, individually) from the config file.

An additional method for on_player_joined event is also implemented, tho not used, for future edge cases we might encounter.

Additionally, 2 commands have also been added to provide support for single-users and RedMew servers CLI:

  • /permissions-reset - reset all users permissions to default (admin/server only)
  • /permissions-set-scenario - to reapply the scenario's restrictions, if any (admin/server only)

This should give us plenty of room to expand for future applications, has global ON/OFF from config file so it's easy to manipulate, quick to use from scenario templates, & backed by admin commands in case emergency actions need to be taken. From in-game, & remotely.

@RedRafe RedRafe requested a review from grilledham January 11, 2024 18:22
Copy link
Collaborator

@grilledham grilledham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to do this, very much appreciated. The commands are a nice touch. However, I do have some thoughts/question I'd like you to answer.

config.lua Show resolved Hide resolved
features/permissions.lua Show resolved Hide resolved
end

---Init permissions
Event.on_init(function()
Copy link
Collaborator

@grilledham grilledham Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked the idea of the permission only be disabled when running on the server. Easier for players who want to play single player to not have to configure anything. I think we could have a flag in the config that determines which event we use, either on_init or on_server_started. It could even default to on_server_started given that is our only use case so far.

I did see your comments in discord, but I think it'll be easier to have the conversation here.

Copy link
Contributor Author

@RedRafe RedRafe Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, im not excluding I've fucked up conditions or events after playing around with all these and/or true/false all day, it's getting me headache

when using on_server_started it also fired when playing the map solo, launched from the new map menu. So I completely abandoned that route because that event didn't do anything different from basically using on_init

For the initialization of permissions, I may have inverted the conditions:

on_init , if

  • Public.enabled() --> enable == true AND presets.any(true)
  • AND not data.initialized

-> this will fire on hosted RedMew servers with scenarios that need this feature
-> this will NOT fire on hosted RedMew servers with scenarios thet dont need this feature
-> SP dowloading from redMew can simply turn it ON/OFF from config file

on_player_joined, if

  • Public.any_preset() --> presets.any(true), regardless of enabled
  • AND not data.initialized
  • AND #players > 1

-> this acts as a backup: if player downloads it from GitHub, it won't fire when playing solo, but the permissions would still apply if playing it in multiplayer

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on_server_started shouldn't fire in single player. The redmew server triggers the event. Do you also see the map poll in single player?

Thinking about this again. I think the distinction between single player and on the redmew server I was asking for was wrong. For the map poll that makes sense as the restart map functionality only works on the redmew server. What I think I actually want is between single player and multiplayer (as in would also apply for other non-redmew servers).

There is a game.is_multiplayer we can use for that, so we can just put that in the on_init function. If a player downloads a multiplayer map and plays in single player, I would think keeping the permission is the right thing to do. And if they start the scenario in single player then the permission should be untouched. We can still put a config flag for is_multiplayer_only (or something similar) that defaults to true.

I don't like the on_player_joined event and don't understand what that gains us. If data.initialized is set in on_init then on_player_joined would never do anything. I think we should just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahah I had the same exact thinking! But unfortunately, is_multiplayer is not available on_init as when creating a map, Factorio doesn't know if you're creating it for solo or multiplayer, even when hosted on our servers for example when the maps gets initialized in a server (that's why S30 had some reset notification the other day while I was testing exactly this route).

So on_player_joined is the substitute for game.is_multiplayer(), that's why I only do that when # of players is > 1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_multiplayer is not available on_init

Crazy, but I can confirm with my own testing that that is correct.

Can we use game.is_multiplayer() inside on_player_joined_game event? I think I'd prefer that over table_size(game.connected_players) > 1 as it's clearer what is being checked and 1 player in multiplayer should still be seen as multiplayer. If we go that route then we don't need the on_init event, just use on_player_joined_game for both cases. Maybe also add a comment that game.is_multiplayer() doesn't work for on_init.

I'd still like to see a flag for multiplayer only on the config. I think in the case a player start the map in single player and moves it to multiplayer we shouldn't change the permissions. The config flag would provide a way to solve that problem for cases where a player really does want to make the map in single player then host it for their own local multiplayer.

Copy link
Contributor Author

@RedRafe RedRafe Jan 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically we can get rid of on_init & the multiplayer setting completely, do (as you're suggesting)

on_player_joined: if config.enabled & is_multiplayer then [...]

This will cover all cases:

  • enabled turns on/off globally
  • only fires on servers
  • single players won't have any issue when downloading

Sounds right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is good and I'd be happy with that.

Maybe it lacks a config option if players still want the permissions set for single player, but they could run the command, so it's ok.

features/permissions.lua Show resolved Hide resolved
map_gen/maps/danger_ores/config/actions.lua Outdated Show resolved Hide resolved
Comment on lines +24 to +26
[defines.input_action.open_blueprint_library_gui] = false,
[defines.input_action.open_blueprint_record] = false,
[defines.input_action.upgrade_opened_blueprint_by_record] = false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the functionality is good now. But I have a questions over which permissions we disable.
Why do we disallow the blueprint library?
I'm not sure what open_blueprint_record and upgrade_opened_blueprint_by_record do?
The preset is really more no_blueprint_imports rather than no_blueprints at least that was my understanding as to what we were trying to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main objective was stop players spam "illegal" BPs, without running a check on BPs themselves.

bp_imports is to stop players import new BP strings

open_bp_recoprsis to stop players accessing BPs already saved in their blueprint books

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for the clarity.

@grilledham grilledham merged commit 579dad4 into Refactorio:develop Jan 14, 2024
1 check passed
@RedRafe RedRafe deleted the permissions-module branch January 18, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants