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

Allow fake players to be given stages. #14

Merged
merged 2 commits into from
Apr 23, 2018

Conversation

cpw
Copy link
Contributor

@cpw cpw commented Apr 21, 2018

This should mean that gamestages are much more compatible with automated
miners and such.

You could probably provide a config to whitelist specific fakeplayer ids
rather than all. The only call that won't work is "getAllStages" since
that's not a globally stored list.

This would likely address issues DarkPacks/SevTech-Ages#1733 and DarkPacks/SevTech-Ages#1803

This should mean that gamestages are much more compatible with automated
miners and such.

You could probably provide a config to whitelist specific fakeplayer ids
rather than all. The only call that won't work is "getAllStages" since
that's not a globally stored list.

This would likely address issues DarkPacks/SevTech-Ages#1733 and DarkPacks/SevTech-Ages#1803
@cpw
Copy link
Contributor Author

cpw commented Apr 21, 2018

My opinion is that gamestages should not apply to fake players. They're not really exposed through the usual mechanisms, you can't interact with them with commands for example.

You could perhaps have two policies "all" and "none" - and fakeplayers could be swapped from one policy to another if you wanted. Trying to restrict fakeplayers to the same system players have would require a significant rewrite of how fakeplayers themselves work, and it would result in a lot of weirdness and confusion.

@AlexIIL
Copy link

AlexIIL commented Apr 21, 2018

Sorry, @justinrusso asked for my recommendation on this and I never actually replied to DarkPacks/SevTech-Ages#1733 so this seems like a good place.

I'd recommend separating player game data from the EntityPlayer itself, and use a separate system for saving and loading. Obviously it's quite a large change to move save data from player entity to somewhere else, but it should be possible. This would be useful for the case where the machine stores the UUID(or GameProfile) of the player who placed the block, like with buildcraft.

This doesn't work for mods that don't know the player who placed them (or which don't store it yet), however I think that it would still be a useful compromise. It's not a lot of data to just store the UUID of the placing player (and I think this is recommended for forge's permission system?). You can always fall-back on accepting or always denying actions, but it might break progression a bit if one automated miner can mine everything and another one requires the player to go through the stages.

Basically, I'd only recommend merging this if moving save files over (or looking up the real, currently loaded, EntityPlayer instance and inspecting that) isn't a valid alternative.

@cpw
Copy link
Contributor Author

cpw commented Apr 21, 2018

So, @AlexIIL the problem is that player context is generally not available to automated machines, and few actually support storing and propagating it. The primary purpose of gamestages is to give a progression to real players, not buildcraft quarries. The progression element should be access to the quarry machine, not what the quarry can do, in my opinion.

@AlexIIL
Copy link

AlexIIL commented Apr 21, 2018

few actually support storing and propagating it.

This is a very small amount of data to store, and adding support for it really shouldn't be tricky, if it is actually used.

The primary purpose of gamestages is to give a progression to real players, not buildcraft quarries. The progression element should be access to the quarry machine, not what the quarry can do

I haven't used this mod yet, so this might not apply: I'm assuming that it's possible to disallow breaking a block until a certain stage, which implies that nothing you can do can break a block. (Except perhaps TnT that will just destroy it, but that's a little different that obtaining it). With this PR you could unlock the given gamestage, or place a quarry in order to break the block. This would mean that the game stage containing the quarry would need to be after every single stage that unlocks a block.

in my opinion.

I can't really argue with an opinion, as @Darkhax's opinion on this is probably more important than mine.

(sorry for double-post, that was an accident)

EDIT: The progression element might also apply to crafting recipes: if a recipe is gated behind a game stage then an automated crafting table could work with this as well.

@justinrusso
Copy link

It's not a lot of data to just store the UUID of the placing player (and I think this is recommended for forge's permission system?)

I am all for getting mods to follow standards. If this is true and is a recommended thing mods should be doing, I am not against complaining to mod devs to add that support of they are not.

In RecipeStages we currently use a setContainerStage method that defines set stages that container will support. From there, when the player attempts to craft something, it will check if the player has that stage and only show an output if true. I feel like comparing between the two, @AlexIIL 's method to allow the miner to adapt would be more ideal than just a bypass switch as this would allow players to obtain blocks they shouldn't because of that bypass.

@AlexIIL
Copy link

AlexIIL commented Apr 21, 2018

If this is true and is a recommended thing mods should be doing, I am not against complaining to mod devs to add that support of they are not.

I'm originally referring to something that I think Lex mentioned in the various permission API PR's and issues. However until I can find the source (and I haven't yet) I don't think it can be considered a standard or official recommendation. Sorry. However it still provides one way for mods to work with this, so it's not a complete waste.

EDIT: although I'll note that the main PermissionAPI file does essentially say this in the javadoc for the GameProfile argument.

from a config file: gameStagesFakePlayerData.json. See https://gist.github.com/cpw/1fa28709c8f1b0c73a430fb8d1029879
for example config file. (Non-existent by default).
@cpw
Copy link
Contributor Author

cpw commented Apr 21, 2018

So I updated my commit after talking to darkosto about how he thinks this should work.

It is basically a fixed policy for each fakeplayer - they get a list of gamestages permanently. I think this is "minimum base functionality" to get fakeplayers to work well enough for people like Darkosto.
Gist example "config" file is here: https://gist.github.com/cpw/1fa28709c8f1b0c73a430fb8d1029879

On the idea of trying to get every other mod in the world to track and supply placing players correctly - the reality is that that is extremely unlikely to ever happen. For those that do do something better - expose an API via IMC for them to call to register their fakeplayer properly.

Commands can probably tweak the fakeplayers as well, if you think it necessary (so you could extend the existing command set to grant new stages to fake players).

@AlexIIL
Copy link

AlexIIL commented Apr 22, 2018

That's a good middle ground, but it probably won't handle Buildcraft type of game profile saving very well - as for us, the player name is used rather than sometime like "[buildcraft]". (Which will just fallback to the default).

That being said I probably won't be implementing my own suggestion as a PR (due to its complexity).

@cpw
Copy link
Contributor Author

cpw commented Apr 22, 2018

The fakeplayer name is just a string - and it's the same field as the player's name. the rftools builder for example has the name "rftools_builder".

so you'd have an entry in the config file with that name, and gamestages to apply to the fakeplayer.

If you don't want commands to interact with the fakeplayers (they're not really visible to the commands at present, because fakeplayers are never actually on the server's playerlist) just leave it as is.

Also note that there is no "saved" profile for a fakeplayer - that's a big part of the problem right now - no fake player will ever show up on disk with inventory etc - that's not how fakeplayers work. That's why this PR skips attaching the capability at all - there's nothing to save or load.

@Darkhax
Copy link
Member

Darkhax commented Apr 22, 2018

Thanks for making this PR and starting this discussion. I have been planning to do a rewrite to improve the internals for a while now. I opened up this issue so people can discuss the rewrite in one place.

As for this PR, I think this is a good solution without breaking compatibility although I am curious to @AlexIIL 's response. There should be some way to identify a Buildcraft FakePlayer from a normal one, perhaps by class name?

@AlexIIL
Copy link

AlexIIL commented Apr 22, 2018

@Darkhax this PR perfectly addresses the case that cpw is talking about (as mods don't give enough information to work with), and #15 should address Buildcraft's case properly. So I don't recommend trying to identify the Buildcraft fake player (or any others that same the full player name) alongside this.

(Although yes, currently you can identify the Buildcraft fake player by class name - however this was only done to override some entity player breaking methods that should be in forge, so I doubt we will keep if forever).

@cpw
Copy link
Contributor Author

cpw commented Apr 22, 2018

@AlexIll one thing - the BC fakeplayer extends the forge fakeplayer so this code will treat the two identically. Fortunately, the days when modders would implement complete fake players are for the most part a historical artifact last seen in 1.7.10.

@Darkhax Darkhax changed the title Make fakeplayers return an "always true" gamestages profile. Allow fake players to be given stages. Apr 23, 2018
@Darkhax Darkhax merged commit 8a2151d into Darkhax-Minecraft:master Apr 23, 2018
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.

None yet

4 participants