keepalive.sc supporting multiple fake-players and flight saving#422
keepalive.sc supporting multiple fake-players and flight saving#422VoidedBlades wants to merge 13 commits intognembon:masterfrom
Conversation
a lot of this could probably be simplified, however in minecraft version 1.21.4 ive noticed that `modify(e, state, value)` doesnt directly do something when applied during the exact timeframe as on_player_connects runs and throws an error inside of `__spawn_players` due to attempting to modify a non-existing entity, a potential race condition happening here. This functions from my testing 100% of the time while retaining positional accuracy on minecraft v1.21.4, tested with 2 fake-players flying at high altitude. im currently unable to test it on prior versions or 1.21.5 due to time constraints so external testing on other version would be greatly appreciated Cheers!
replaceitem
left a comment
There was a problem hiding this comment.
Thanks! Decided to review this since there were multiple people reporting issues with the old scripts in the discord.
| ); | ||
|
|
||
| __on_player_connects(player) -> ( | ||
| data = load_app_data(); |
There was a problem hiding this comment.
Loading this data on every player connection (even non fake ones and after server startup) feels unnecesary. I would suggest loading this data once in __on_connect and storing it in a global variable (ideally converted to a map with the player names as keys for quick access, instead of the foor loop in line 29). Then on every connect you can just check if the connected player's name is in that map. If so, change the gamemode and remove it from the map, so future connections of this player won't have it's gamemode changed.
programs/utilities/keepalive.sc
Outdated
| __on_connect(e) -> ( | ||
| // forcing the fake-players that were spawned in spectator back into creative to retain flight per their data | ||
| if(e:'gm' == 'creative' && e:'fly' == 1, | ||
| sleep(50); // the direct delay is needed to force it back into the proper gamemode. it cannot for the life of it do so at the exact same moment and requires the delay. |
There was a problem hiding this comment.
Using sleep and running this in a task might introduce other race conditions. I would suggest just running it at the end of the tick with schedule(0, ...) and not in a task
programs/utilities/keepalive.sc
Outdated
| for([str('gamemode %s %s', e:'gm', e:'name')], | ||
| logger('warn', _); | ||
| run(_); | ||
| ); |
There was a problem hiding this comment.
For running this single command a loop is not neccesary, and in general, I don't really see a need to also log the command. In my opinion it would suffice to use modify(..., 'gamemode', ...)
programs/utilities/keepalive.sc
Outdated
| if (_:'gm' == 'creative' && _:'fly' == 1, | ||
| // spawning creative fake-players in spectator to force the flying state as using modify throws an error due to potential race conditions | ||
| for([str('player %s spawn at %f %f %f facing %f %f in %s in spectator', _:'name', _:'x', _:'y', _:'z', _:'yaw', _:'pitch', _:'dim')], | ||
| logger('warn', _); | ||
| run(_); | ||
| ); | ||
| , _:'gm' != 'creative', | ||
| for([str('player %s spawn at %f %f %f facing %f %f in %s in %s',_:'name', _:'x', _:'y', _:'z', _:'yaw', _:'pitch', _:'dim', _:'gm')], | ||
| logger('warn', _); | ||
| run(_); | ||
| ); | ||
| ); |
There was a problem hiding this comment.
This is nearly the same thing twice, use a temporary variable for the gamemode and use that in the command (overwrite it to spectator if flying).
The for loop is not needed here.
These two if conditions do not cover non-flying creative players, which makes them be ignored and not respawned at all.
|
@replaceitem Ive looked over your suggestions and ive implemented them to the best of my ability as im quite new to the scarpet language. This should cover most if not all if im correct. I have however changed up something slightly. instead of initializing and caching the data in I reckon there may be potential to re-introduce the modification towards the flight variable directly with Edit: after having tested it, this is a viable approach with the current setup. although not extremely extensively tested, it spawned the fake-player in creative with flight and no errors from what i could tell |
|
Oh sorry - yeah - reading and caching it on server startup is what i meant, not in |
programs/utilities/keepalive.sc
Outdated
| player_name = player()~'name'; | ||
|
|
||
| if (has(global_cached_players, player_name), | ||
| task('__on_connect', global_cached_players:player_name) |
There was a problem hiding this comment.
Don't see a reason to use task - theres no expensive conputation or sleep inside. It schedules immediately anyway. In that case you could also just move the contents of __on_connect into the player_connects event directly.
In fact if using the modify(e, 'flying' approach (which i prefer), using a task here could lead to some delay before setting the flying, during which the player could be ticked, causing the player to drop down a bit.
I would consider removing the task for __spawn_players in __on_server_starts too. This seems to have been a fix for this script previously (#410) seemingly not working for everyone. Since this should now be fixed fully by waiting for the player to be spawned with the event, it should be fine to remove.
There was a problem hiding this comment.
A quite curious thing is happening if i remove the task directed towards __on_connect and put the contents directly into the function. It seems that modify doesn't actually function accordingly in this instance. you are right in that we can remove __spawn_players' task however this specific task in __on_player_connects seems to be required to stay unless we have any other viable options for replacement.
There was a problem hiding this comment.
In my attempts, replacing task('__on_connect', global_cached_players:player_name); with __on_connect(global_cached_players:player_name); worked fine. As long as there is still the schedule(0,...) in there, it will run the modify on the main thread (end of tick, after the player fully connected) anyway.
There was a problem hiding this comment.
interesting, last time i checked i removed __on_connect to move the code directly into __on_player_connects which didnt function, it did just now when i checked
| __on_player_connects(player) -> ( | ||
| player_name = player()~'name'; | ||
|
|
||
| if (has(global_cached_players, player_name), |
There was a problem hiding this comment.
After checking this, i would suggest removing the entry from the map too. After all the player might be spawned multiple times in different gamemodes manually later.
For example:
/player test spawn in creative(Spawn flying player in the air)- Reopen server, script spawns player back in creative and correctly sets flying to true
/player test kill/player test spawn in survival
Now you got a flying player in survival, since it still saved that test should be flying
There was a problem hiding this comment.
rather than removing the map i reckon it may be better to clear the key from the map on __on_player_disconnects?
this would support utilizing the map for any future usages where needed alongside removing the obstructions mentioned.
Ive tried moving the task into __on_server_starts as well as a hopeful functioning alternative but alas. it has to be through __on_player_connects of sorts
There was a problem hiding this comment.
That would work too, though i don't see any use for that currently.
Do you have any more details on why modify is not working if you use a task? Is the player not available?
There was a problem hiding this comment.
the player is not available during __on_server_starts but afterwards during the __on_player_connects it is obviously, no errors seem to happen even removing the task inside of __on_player_connects however when entering the server the actual modification hasnt happened without said task and the player is on the ground but in creative.
im assuming this is due to some sort of race condition between a form of post initialization of the player and the value modified, in this case the flying.
its an incredibly peculiar situation nontheless.
There was a problem hiding this comment.
Just to confirm, you are still using the schedule(0, _(e) -> (modify(player(e:'name'), 'flying', e:'fly')), e); right?
It's true that the flying state of the player is set after the player_connects event is fired, which would overwrite that if modify was used directly in the event. Scheduling to the end of the tick should fix that though.
As far as i can tell, there is no asynchronous initialization of the player, it all happens right after.
There was a problem hiding this comment.
ive pushed a new update, its still using schedule and at the time was too to modify it so everything should be fine now i reckon? do let me know if anything else needs adjusting!
|
Tested it in both singleplayer and on a server and seems to work well now. |
|
It seems to still have mixed tabs and spaces |
a lot of this could probably be simplified, however in minecraft version 1.21.4 ive noticed that
modify(e, state, value)doesnt directly do something when applied during the exact timeframe ason_player_connectsruns and throws an error inside of__spawn_playersdue to attempting to modify a non-existing entity, a potential race condition happening here.These changes function from my testing 100% of the time while retaining positional accuracy on minecraft v1.21.4, tested with 2 fake-players flying at high altitude.
im currently unable to test it on prior versions or 1.21.5 due to time constraints so external testing on other version would be greatly appreciated.
Cheers!
fixes #408 fixes #388