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

Stairs: Lengthen interval of replace abm #604

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@paramat
Copy link
Member

commented Aug 2, 2015

The interval is currently insanely frequent at 1 second. Because ABM range is 32 nodes, assuming a player walking past a node that needs replacing, the node is within range over a 64 node walk, at 4m/s this means a 16 second interval is all that is needed.

@PilzAdam

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2015

This also means that the player sees an unknown node for up to 16 seconds.

@C1ffisme

This comment has been minimized.

Copy link

commented Aug 2, 2015

I think 16 seconds is a good amount of time to wait if you have a warning before you start digging.

For example:

"Old stair nodes were detected in your map!"
"If you find an unknown node, wait around 20 seconds before digging it. It might be a stair node!"
"Old stair nodes are replaced every 16 seconds, so wait before digging!"

But only print this if old stairs are detected. (No idea how you would do this, though.)

@paramat

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2015

Aha okay. Let's consider a player walking up to a node, for it to convert before arriving at a node an interval of 8 seconds is needed. ABM checks are heavy and i feel it's more important to lengthen intervals than to immediately hide an unknown node from view at a distance. The ABM will currently be running and scanning map every second even after all nodes have been converted.
If the node is seen from a distance and the player walks up to it to inspect, it will convert before the player arrives.
8 seconds okay? I will update the PR.

@PilzAdam

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

I think increasing the interval here to gain a performance boost is not good. From a mod perspective, the ABM is executed very rarely, since the node it runs on is replaced.

I think the engine needs to be tweaked to not spend too much time finding nodes for ABMs that run on very uncommon nodes.

EDIT: Also mods shouldn't depend on implementations of the engine. The engine might implement ABMs by keeping references to all nodes that it should run on; in this case this ABM would be basically a no-op when no old stairs are around (which is true 99.9% of the time).

@paramat

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2015

The ABM is executed rarely, but the ABM search still runs when there are no nodes to convert, once per second is overkill and not justifiable. Even if ABM search is improved thiis conversion is of low importance so should have the longest reasonable interval of 8 seconds.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2015

Approval from sfan5 if updated to 8 seconds http://irc.minetest.ru/minetest-dev/2015-08-03#i_4350017

@paramat paramat force-pushed the paramat:stairsabm branch from 6ed11f1 to 7dcc30f Aug 3, 2015

@paramat

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2015

@paramat paramat closed this Aug 4, 2015

@paramat paramat deleted the paramat:stairsabm branch Aug 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.