mt-mods / pipeworks

Pipeworks is a mod for Minetest allowing the crafting and usage of pipes and tubes
Other
14 stars 17 forks source link

add dummy implementations of all current API calls to fake player #98

Closed fluxionary closed 6 months ago

fluxionary commented 7 months ago

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 https://github.com/minetest/minetest/issues/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?

wsor4035 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 :thinking:

S-S-X commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

S-S-X commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

fluxionary commented 7 months ago

should i put all the stateless functions in a metatable

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

BuckarooBanzay commented 7 months ago

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