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

Re-save active entities more often if they move a certain distance #15605

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andriyndev
Copy link

@andriyndev andriyndev commented Dec 30, 2024

This patch should fix a bug when location in the database might be not updated in case of server crash, if the location in the database belongs to a block which wasn't unloaded up until the crash. The bug was fixed by adding a check of the distance between the two locations, and updating the data in case if it reached some threshold (MAP_BLOCKSIZE was used but it could be reconsidered.

To do

This PR is Ready for Review.

How to test

  • Create a server with the paleotest mod installed (https://content.luanti.org/packages/ElCeejo/paleotest/)
  • Run the server in the multiplayer mode, log in as two separate players
  • One player places "Spawn Quetzatcoatlus" very close to the second one, it should spawn an adult Quetzalcoatlus
  • The first player mounts the Quetzalcoatlus by right-clicking it with "Whip", and flies away ("Space" to ascend, "S" to descend, mouse to turn). The second player should remain in the same place to keep the area loaded
  • The first players flies away on a large distance (I tested with the distance of 2000 blocks but in theory it could be even two opposite ends of the world)
  • The server is killed with SIGKILL (pkill -9 luanti)

With this patch, after next log in the position of the mob should be near the position of the first player (as expected by users). Without this patch, the mob will be at the place where it was initially spawned, i.e. near the second player.
Remark: the latest version of the mod is buggy, and the mob might die for no reason during descend, but in other aspects it seems to work OK, and it's one of the easiest way to test the scenario I'm aware of.

@andriyndev andriyndev marked this pull request as draft December 30, 2024 21:56
@andriyndev
Copy link
Author

andriyndev commented Dec 30, 2024

The current version is incorrect, since I accidentally compare the blocks as if they are coordinates, and we seem to could also use obj->getMinimumSavedMovement() for the threshold. I'll rewrite it tomorrow, and also investigate the surrounding code in more details to fully understand how it works.

@sfan5
Copy link
Collaborator

sfan5 commented Dec 30, 2024

I wouldn't call this a bug per-se since there are always situations where you can kill the server and certain progress will be missing.
However this is a situation where we can easily improve it.

we seem to could also use obj->getMinimumSavedMovement() for the threshold.

that's probably less than a block size (16) so not directly usable here.

if we respected this value properly that'd mean we re-save the object every block (even tho the old is still active). that doesn't seem very sustainable, so might be a good idea introduce a new magic value like e.g. 6 blocks movement.

@andriyndev
Copy link
Author

Changed the size, so that the entity will be resaved when there is one block between the block with old (saved) position and the block with the new position (the distance between blocks is >= 2 where 1 is the width of one block). I didn't choose neighboring blocks, otherwise it'll be re-saved each time the entity crosses the border between them. I decided to use block coordinates, since we don't have the old (saved) coordinates at hand, and we'll need to search for the object in the block as it's done in the code below. I specified the size 2 directly in the check, however, it may be better to specify it as a separate variable, property or macro.

@andriyndev andriyndev marked this pull request as ready for review January 5, 2025 18:24
src/serverenvironment.cpp Outdated Show resolved Hide resolved
src/serverenvironment.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 changed the title Fix a bug when entity's location might not be updated in case of server crash Re-save active entities more often if they move a certain distance Jan 6, 2025
@tigercoding56
Copy link

can we make the distance that is needed for re-save a settings option ?

@appgurueu
Copy link
Contributor

This is trading one consistency issue (player position being updated while entity position is not) for another (some entity positions get updated, others do not) plus potentially reduced performance if the chosen number turns out unsuitable for servers with plenty of decently fast-moving entities.

(Unfortunately a proper consistency fix for hard crashes is hard because players and entities live in different databases.)

@andriyndev
Copy link
Author

As a mod creator, I agree that it'd be great to give mod developers the ability to customize this. But in this case it's value should be more meaningful, like real distance, because the current approach will, probably, be difficult to explain in the API documentation.
Also, another possible approach, which could also address the problem with consistency between players and world database, is to add a new type of entity which would hardly link to a player, and could store in the database with players (not in the world database) when they are ridden by a player. An advantage of this approach is the possibility to solve the fundamental problem when player logs out or the server crashes while the players ride a mob or some transport, and after logging in it turns out outside of it. It's crucial in case of flying mobs or planes, because in this case the player just falls to the ground and, probably, dies, and it's currently almost impossible to do anything with it (at least I don't know any solution). In case of storing such mobs in the database with players, if the player logs out, the mob could disappear too, and reappear after the next log in already attached to the player again. In this case, we might not even need the current fix, since in other cases this problem is probably not so crucial. The problems of this approach are that it would be probably difficult to implement, we'll have two separate storages for mobs, bugs in mobs could keep players attached to them without possibility to detach, especially if they are glitched into solid nodes, stuch there, and the player needs to right-click the mob to detach (however, this could be solved by implementing an universal way for players to force-detach from such mobs, maybe with a chat command).

@sfan5 sfan5 self-requested a review January 12, 2025 20:38
Copy link
Collaborator

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Looks good but I think a bit higher limit is appropriate

@@ -43,6 +43,8 @@
// A number that is much smaller than the timeout for particle spawners should/could ever be
#define PARTICLE_SPAWNER_NO_EXPIRY -1024.f

#define ACTIVE_OBJECT_RESAVE_DISTANCE_SQ 2 * 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#define ACTIVE_OBJECT_RESAVE_DISTANCE_SQ 2 * 2
static constexpr s16 ACTIVE_OBJECT_RESAVE_DISTANCE_SQ = 3 * 3;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants