Fix active object adding to not generated block

[sfence]

• Goal of the PR
  Fix minetest.add_item and minetest.add_entity are unreliable #4759.
• How does the PR work?
  It disables saving Active objects to a blank map block, so the error is not masked.
• Does it resolve any reported issue?
  Fix minetest.add_item and minetest.add_entity are unreliable #4759.

To do

This PR is a Ready for Review.

How to test

Try to spawn an entity with the command /spawnentity in not generated part of the map and teleport to that part.

[Zughy]

(Unrelated: have you considered proposing as a core dev?)

[lhofhansl]

I looked through the issue. Is it actually a good idea to allow entities (items, objects) for non-existing blocks?

Edit: Oh wait. This is not allowing that, this is about add_entity/add_item to return consistent information.

[appgurueu]

Tested.

On master, you simply get an (invalid?) object ref; /spawnentity testentities:selectionbox 1000,1000,1000 does not complain, but when you /teleport 1000,1000,1000, the entity isn't there. There's clearly a bug here.

With this PR, you get an "ERROR[Server]: ServerEnvironment::addActiveObjectRaw(): could not emerge block for storing id=5 statically (pos=(1000,1001,1000))" (?), but also an invalid object ref. I'm not sure that's what we want; what's the expected behavior here? Do we want adding the object to fail (loudly) - as opposed to adding the entity, such that it is there when the mapblock is generated - do we want a (somewhat weird) error message for that?

[sfan5]

FWIW saveStaticToBlock does effectively the same, so the fix would need to be applied there too.

Actual question is however: why doesn't this work right now?
If I spawn an entity far away the engine creates a blank block, saves the static data there (initially), the entity is soon deactivated and the static data should be written there again.
If I visit the block sure the mapgen might run but the entity should be there.
-> more debugging needed, this is maybe not the right fix

With this PR, you get [an error], but also an invalid object ref.

This should logically not happen. Are you sure you tested right?

[appgurueu]

This should logically not happen. Are you sure you tested right?

Sorry, I should have written a nil value. The object ref is in the former case.

[sfence]

It looks like active objects created in nongenerated areas are stored well.
But when the block is generated, this line is called:

minetest/src/serverenvironment.cpp

Line 1055 in cb689b5

block->m_static_objects.clearStored();

Context:

minetest/src/serverenvironment.cpp

Lines 1053 to 1057 in cb689b5

	// Remove stored static objects if clearObjects was called since block's timestamp
	if (stamp == BLOCK_TIMESTAMP_UNDEFINED || stamp < m_last_clear_objects_time) {
	block->m_static_objects.clearStored();
	// do not set changed flag to avoid unnecessary mapblock writes
	}

[sfence]

This is probably source of the problem:

minetest/src/mapblock.h

Lines 531 to 535 in cb689b5

	/*
	When block is removed from active blocks, this is set to gametime.
	Value BLOCK_TIMESTAMP_UNDEFINED=0xffffffff means there is no timestamp.
	*/
	u32 m_timestamp = BLOCK_TIMESTAMP_UNDEFINED;

Because a nongenerated block is saved before it is activated and removed from active blocks.

[sfence]

So, the timestamp of the block is updated when object status data is saved to the block in addActiveObjectRaw now.
Looks like adding objects to the nongenerated area works now.

[sfan5]

I see a conceptual problem here: Everytime a block is loaded without running through ServerEnvironment::activateBlock the "remove objects if they should be" logic is skipped. If static objects are then modified you risk them getting deleted.

e.g. following case: block is activated and saved once, clearobject called, block is loaded to add new static object, block is loaded for real afterwards -> new object deleted too

However we can't just increase the timestamp to fix it because the delta is also used by other code.
Fixing this is not possible without somehow separating the "clearobjects detection value" from the timestamp.

Anyway as the currently best fix I suggest not calling clearStored when the timestamp is undefined. (I don't see the logic in that anyway.)

[sfan5]

works

[rubenwardy]

Please can you amend lua_api.md to define what happens when adding to a not generated/loaded area?

stale

[appgurueu]

I asked over on the issue what the expected behavior is, and here's Wuzzy's reply: #4759 (comment) - it seems not spawning the object is considered expected behavior by Wuzzy, and he makes a good point: We probably don't want active objects in unloaded areas.

We could have a second variant of add_entity, which adds objects to unloaded areas (or rather, schedules them to be spawned when the block is generated), but doesn't return an active object ref.

[sfence]

@appgurueu Something as is done in the last commit, can be?

To test both modes, change line

minetest/builtin/game/chat.lua

Line 892 in ca0a57d

local obj = core.add_entity(p, entityname)

to local obj = core.add_entity(p, entityname, "", true).

[sfan5]

What's with all the unnecessary changes? This PR is only getting further from a mergeable state (IMO).

All the modder wants to do is spawn an entity. Then we add a check that prevents spawning it. But in case the modder really wants to he can tell the engine to "actually do it anyway" with a new parameter.
I don't know why Wuzzy only wants to spawn entities in loaded blocks. But if that's a concern he can use minetest.compare_block_status or even minetest.get_node_or_nil to check before spawning.
There is no need to complicate it for everyone else.

[sfence]

Looks like adding two modes is controversial.
The last 3 commits reverted.

Now add_entity will work for loaded, unloaded, and nongenerated blocks as well.

[sfan5]

We probably don't want active objects in unloaded areas.

btw this isn't as problematic as it sounds.
What happens is that the entity is spawned as active object and an ObjectRef returned. Quickly afterwards (not sure if on the same step, but at earliest on the next step) it is unloaded and saved as a static object to disk.

Consider that objects can also teleport or move right into unloaded blocks.

[grorp]

I've tested that this PR fixes the test case provided in #4759 (comment) by Wuzzy. It also correctly returns errors for entities outside the map boundaries.

I extended the test code as follows: https://sprunge.us/MKuZU6?lua. When activating all entities two times by flying back and forth along the line generated by /spawntest, this was the output:

Server for gameid="minetest" listening on 0.0.0.0:30000.
grorp [127.0.0.1] joins game. List of players: grorp
[on_activate] got right staticdata for dtime_s == 0 (×1000)
2024-02-01 14:56:38: ERROR[Server]: minetest.add_entity reported 1000 succeeded spawns.
2024-02-01 14:56:38: ERROR[Server]: minetest.add_entity reported 0 failed spawns.
[on_activate] got wrong staticdata for dtime_s == 0: "later staticdata" (×932)
[on_activate] got right staticdata for dtime_s != 0 (×1032)
grorp leaves game. List of players: 
Server: Shutting down
2024-02-01 14:58:42: [Main]: INFO: signal_handler(): Ctrl-C pressed, shutting down.

The [on_activate] got wrong staticdata for dtime_s == 0: "later staticdata" (×932) first confused me, but now I realize it makes sense: The block containing the object is not generated yet, the block timestamp is BLOCK_TIMESTAMP_UNDEFINED and thus dtime_s is 0 when activating the object.

[sfence]

(Unrelated: have you considered proposing as a core dev?)

(Unrelated: Have no idea what responsibilities come with it.)