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 dummy implementations of all current API calls to fake player #98

Merged
merged 3 commits into from
Dec 29, 2023

Conversation

fluxionary
Copy link
Member

@fluxionary fluxionary commented Nov 24, 2023

this is an alternate to the proposal in #18.

the incompleteness of the fake player's API causes crashes in unsuspecting mods, making it a huge headache for large servers with large numbers of mods. most recently for the your-land server, this was due to ts_furniture trying to call get_player_velocity(). the "expected" solution is to add a check for player.is_fake_player, but i find this to be a terrible solution, as this isn't part of the published minetest API (though luanti-org/luanti#11477). mods that have no dependency or awareness of pipeworks shouldn't be required to conform to the non-standard assumptions it imposes. it's also difficult to get all mods to "fix" their code that way, because a lot are abandoned, and some (e.g. petz) won't add such a check.

so, i've created dummy functions for all the API calls currently in ObjectRef. i've added no extra state to the fake player - trying to modify various things is usually just a no-op. the results may not always be what the mod expects but at least the crashes will go away.

one downside of this is that the fake player API will need to be updated whenever the real player API changes.

i've got a snippet that i used to generate the list of API calls that weren't yet implemented, possibly that should be added to help w/ future updates, though i'm at a loss for the best place to put it.

i'm aware that digtron has a similar problem, though i'm less concerned about it. but perhaps instead i should create a fake player mod that pipeworks and digtron could both rely on?

Copy link
Contributor

@wsor4035 wsor4035 left a comment

Choose a reason for hiding this comment

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

looks ok, but probably should be tested with other mods to make sure nothing..... implodes. could try a pr to dreambuilder with this branch so that its ci runs.......

@wsor4035
Copy link
Contributor

relating to abstracting a mod out for this, rather leave it here so its easier/faster to fix in the context of the engine already has some issues about handling fake players, so would be best off handled there

@OgelGames
Copy link
Contributor

OgelGames commented Nov 25, 2023

create a fake player mod

Funny, I was just working on that the other day, trying to finish one of the many projects I have :P

It's not just pipeworks and digtron that need fake players, there are other mods that need them too, such as digibuilder, which has a copy of the pipeworks code: https://github.com/BuckarooBanzay/digibuilder/blob/master/create_fake_player.lua. Creating a fake player mod means there will be only one version to maintain, assuming other mods use it.

Regarding this PR, there is something else to consider: adding all these extra functions significantly increases the time it takes to create a new fake player (which is already quite slow). It would be better to use a metatable, so the functions are only ever created once, which is the method I'm using.

@BuckarooBanzay
Copy link
Member

adding all these extra functions significantly increases the time it takes to create a new fake player (which is already quite slow)

Another alternative would be to cache them and evict periodically, they don't have internal state or anything 🤔

@S-S-X
Copy link
Member

S-S-X commented Nov 28, 2023

Should you also override core sound functions and add check for fake player or are those already fine with fake player?

I think there there was some other similar core functions that can't tolerate fake player instances.

Then of course there's few core functions that could be overridden to improve compatibility but in most cases probably better to change mod code instead, for example things like getting player by name and but like said I think in most cases probably better to actually ask others to keep their player instance instead of losing it and getting it again by name.

@fluxionary
Copy link
Member Author

so this has been approved, but there's suggestions about possible improvements. should i put all the stateless functions in a metatable, or leave this PR as is in anticipation of Ogel releasing a better solution? should i check that nothing crashes if the fake player is passed as an argument to various API calls? the only things i can see that take a player argument that i could override are check_player_privs and calculate_knockback, and those work fine w/ the fake player. ObjectRef:punch(puncher, ...) crashes if the puncher isn't an ObjectRef, but i can't override that (i don't think?). are there other cases i'm not seeing?

@wsor4035
Copy link
Contributor

if you assume https://stackoverflow.com/a/18797720 is correct, seems dreambuilder works fine with this mt-mods/dreambuilder_game@b620e8e

@S-S-X
Copy link
Member

S-S-X commented Nov 29, 2023

if you assume https://stackoverflow.com/a/18797720 is correct, seems dreambuilder works fine with this mt-mods/dreambuilder_game@b620e8e

StackOverflow is right but you have to remember that you still need to commit update submodule hash. They're not talking about automatically pointing to submodule remote branch latest commit but about about automatically updating to remote latest commit, these are different things as former would not need additional actions from you but latter does need commit to update actual submodule commit pointer.

In other words it does not follow but instead lets you know what to follow.

@OgelGames
Copy link
Contributor

should i put all the stateless functions in a metatable, or leave this PR as is in anticipation of Ogel releasing a better solution?

Preferably the first option, because I don't know when I'll have time to finish my solution, and it's better to have some improvement over nothing.

@SwissalpS
Copy link
Contributor

keep in mind, during testing, that fakeplayer may be used while corresponding real player isn't online.

@BuckarooBanzay BuckarooBanzay added the enhancement New feature or request label Dec 3, 2023
@fluxionary
Copy link
Member Author

should i put all the stateless functions in a metatable

whoops, did this a while back but forgot to commit/push it.

@BuckarooBanzay
Copy link
Member

IMO: this could be merged and enhanced (or moved out to a separate mod) later on

common.lua Outdated Show resolved Hide resolved
@Niklp09 Niklp09 self-requested a review December 20, 2023 06:00
@wsor4035 wsor4035 merged commit 5b0dceb into master Dec 29, 2023
2 checks passed
@Niklp09 Niklp09 deleted the fix_fakeplayer branch December 29, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants