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

Improve active object behavior - extend active object in view direction. #6667

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@lhofhansl
Contributor

lhofhansl commented Nov 22, 2017

See #6642

Description Updated:
Extend the range in the players direction in which active objects stepped at the server.

Note: active objects are still active at a close range (in sphere around the player). At a distance active objects are only active a players view-cone in order to save server resource - 90-99% depending on FOV setting.

Was:
This does two things:

  1. Extend the range in the players direction in which active objects stepped at the server.
  2. Similar to ABM deal with too much load by skipping a few rounds. This should be generally helpful.
@@ -873,10 +907,6 @@ void ServerEnvironment::activateBlock(MapBlock *block, u32 additional_dtime)
elapsed_timer.position));
}
}
/* Handle ActiveBlockModifiers */

This comment has been minimized.

@lhofhansl

lhofhansl Nov 22, 2017

Contributor

Since blocks are more frequently activated only run the ABMs at their step intervals. I think this is better anyway.

@lhofhansl

lhofhansl Nov 22, 2017

Contributor

Since blocks are more frequently activated only run the ABMs at their step intervals. I think this is better anyway.

fillRadiusBlock(pos, active_block_range, m_abm_list);
fillRadiusBlock(pos, active_block_range, newlist);
s16 player_ao_range = std::min(active_object_range, playersao->getWantedRange());

This comment has been minimized.

@lhofhansl

lhofhansl Nov 22, 2017

Contributor

Note that with default settings this is not called.

@lhofhansl

lhofhansl Nov 22, 2017

Contributor

Note that with default settings this is not called.

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Nov 22, 2017

Contributor

Actually now I think the separation of ABMs and active_objects is not good. mobs_redo for example registers ABMs in order for mobs to spawn. Now the further distance is used to see existing mobs, but no new mobs would spawn in the wider area.

Edit: Nah, this is good. Prevents mobs from disappearing.

Contributor

lhofhansl commented Nov 22, 2017

Actually now I think the separation of ABMs and active_objects is not good. mobs_redo for example registers ABMs in order for mobs to spawn. Now the further distance is used to see existing mobs, but no new mobs would spawn in the wider area.

Edit: Nah, this is good. Prevents mobs from disappearing.

@SmallJoker

This comment has been minimized.

Show comment
Hide comment
@SmallJoker

SmallJoker Nov 23, 2017

Member

This feels like quantum mechanics, where you have to look at it to make it work as expected.
What I want to say with that: Objects will only be active as long you watch them, so there's a higher chance for you to get attacked in front than on the back, because the entities will be enabled there.
For some objects it's wanted that they continue working without looking at them (pipeworks, for example), which is changed by this PR, especially when zooming.
Ignoring the FOV impact, separating the ABM and active object list to handle different block ranges is a very good idea, so spawned entities/mobs around the players will stay active in that range, no matter what the player does.
EDIT: TL;DR: ABM and active object list separation is a good idea, but adjusting to FOV doesn't seem to be an optimal solution for me.

Member

SmallJoker commented Nov 23, 2017

This feels like quantum mechanics, where you have to look at it to make it work as expected.
What I want to say with that: Objects will only be active as long you watch them, so there's a higher chance for you to get attacked in front than on the back, because the entities will be enabled there.
For some objects it's wanted that they continue working without looking at them (pipeworks, for example), which is changed by this PR, especially when zooming.
Ignoring the FOV impact, separating the ABM and active object list to handle different block ranges is a very good idea, so spawned entities/mobs around the players will stay active in that range, no matter what the player does.
EDIT: TL;DR: ABM and active object list separation is a good idea, but adjusting to FOV doesn't seem to be an optimal solution for me.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 23, 2017

Member

I agree with SmallJoker.
"The scary monsters will go away if you don't look at them".

Member

paramat commented Nov 23, 2017

I agree with SmallJoker.
"The scary monsters will go away if you don't look at them".

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Nov 24, 2017

Contributor

@SmallJoker @paramat , they would only be de-activated at a distance. Close by they are still subject to the active block range.

There are two ranges:

  1. The active block range. Here ABMs and active objects are active. This is always a sphere around each player.
  2. The extended range for active objects. There only active objects are active and only where the player looks.

So no, they do not all go away when you don't look, and when you're close they be active whether you look or not.

That seems to be a good compromise between function and performance. Within a defined radius (all around) active objects are always active. Past that distance they are active further out (assuming they cannot attack from 6 map blocks away) in the view cone.

Note with standard FOV we save 90% of the extra server work or 99.6% at zoom FOV 15.

Contributor

lhofhansl commented Nov 24, 2017

@SmallJoker @paramat , they would only be de-activated at a distance. Close by they are still subject to the active block range.

There are two ranges:

  1. The active block range. Here ABMs and active objects are active. This is always a sphere around each player.
  2. The extended range for active objects. There only active objects are active and only where the player looks.

So no, they do not all go away when you don't look, and when you're close they be active whether you look or not.

That seems to be a good compromise between function and performance. Within a defined radius (all around) active objects are always active. Past that distance they are active further out (assuming they cannot attack from 6 map blocks away) in the view cone.

Note with standard FOV we save 90% of the extra server work or 99.6% at zoom FOV 15.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 24, 2017

Member

they would only be de-activated at a distance. Close by they are still subject to the active block range.

Yes i know, sorry my comment may have given the wrong impression. I am still unsure about view direction affecting active objects, but don't feel strongly about it.

Member

paramat commented Nov 24, 2017

they would only be de-activated at a distance. Close by they are still subject to the active block range.

Yes i know, sorry my comment may have given the wrong impression. I am still unsure about view direction affecting active objects, but don't feel strongly about it.

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Nov 24, 2017

Contributor

I also found that the active object skipping I added might be too aggressive.

Played around with TNT (thinking that'd be about the worst case), and indeed with a larger blast radius the active object can be busy for 1s or more (that's bad, but that's a different story). With the 5ms time check this would now skip the active object for 200 rounds, or about 20s.

Need to either remove it, or be smarter. Perhaps keep the moving average of the execution time and use that. That same applies to the existing ABM code btw. only the intervals are less critical and longer there.
In fact I see that the wrong dtime is passed the ABMHandler. Maybe I'll fix these in another PR.

Contributor

lhofhansl commented Nov 24, 2017

I also found that the active object skipping I added might be too aggressive.

Played around with TNT (thinking that'd be about the worst case), and indeed with a larger blast radius the active object can be busy for 1s or more (that's bad, but that's a different story). With the 5ms time check this would now skip the active object for 200 rounds, or about 20s.

Need to either remove it, or be smarter. Perhaps keep the moving average of the execution time and use that. That same applies to the existing ABM code btw. only the intervals are less critical and longer there.
In fact I see that the wrong dtime is passed the ABMHandler. Maybe I'll fix these in another PR.

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Nov 24, 2017

Contributor

OK... This version addresses review comments. It also improves the time budget:

  1. Add an limiter to active objects
  2. When ABMs and AOs are over budgets, increase the limiter budget (instead of skipping a number of steps). That makes it more smooth depending on how far over budget the process it.
  3. Do not hardcode the budget limit. Take 1/5 of the allowed budget (the 1/5 is still hardcoded, though)
Contributor

lhofhansl commented Nov 24, 2017

OK... This version addresses review comments. It also improves the time budget:

  1. Add an limiter to active objects
  2. When ABMs and AOs are over budgets, increase the limiter budget (instead of skipping a number of steps). That makes it more smooth depending on how far over budget the process it.
  3. Do not hardcode the budget limit. Take 1/5 of the allowed budget (the 1/5 is still hardcoded, though)
@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 24, 2017

Member
#    From how far clients know about objects, stated in mapblocks (16 nodes).
#    type: int
# active_object_send_range_blocks = 3

#    How large area of blocks are subject to the active block stuff, stated in mapblocks (16 nodes).
#    In active blocks objects are loaded and ABMs run.
#    type: int
# active_block_range = 3

Before a merge this documentation should be updated to explain the new behaviour.
Both set the same (the current default) doesn't change behaviour. I don't have any objection to the PR, just thinking about possible implications of view-determined object activation =)

Member

paramat commented Nov 24, 2017

#    From how far clients know about objects, stated in mapblocks (16 nodes).
#    type: int
# active_object_send_range_blocks = 3

#    How large area of blocks are subject to the active block stuff, stated in mapblocks (16 nodes).
#    In active blocks objects are loaded and ABMs run.
#    type: int
# active_block_range = 3

Before a merge this documentation should be updated to explain the new behaviour.
Both set the same (the current default) doesn't change behaviour. I don't have any objection to the PR, just thinking about possible implications of view-determined object activation =)

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Nov 24, 2017

Contributor

Done.
Is there any point in splitting this into multiple PRs. One for the time budgets changes and one of the extended view range...

Contributor

lhofhansl commented Nov 24, 2017

Done.
Is there any point in splitting this into multiple PRs. One for the time budgets changes and one of the extended view range...

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 25, 2017

Member

That would be ideal but i personally won't insist on it.

Member

paramat commented Nov 25, 2017

That would be ideal but i personally won't insist on it.

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Nov 28, 2017

Contributor

Let's do it one. Easier that way.
Anything I need to do?

Contributor

lhofhansl commented Nov 28, 2017

Let's do it one. Easier that way.
Anything I need to do?

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Nov 30, 2017

Contributor

Rebased.
Also using a moving average now to calculate the time taken in stepping active object, since that can be very bursty.

Should be all good now.

Contributor

lhofhansl commented Nov 30, 2017

Rebased.
Also using a moving average now to calculate the time taken in stepping active object, since that can be very bursty.

Should be all good now.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 30, 2017

Member

BTW #6683 but if you are busy maybe i can fix those.

Member

paramat commented Nov 30, 2017

BTW #6683 but if you are busy maybe i can fix those.

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Nov 30, 2017

Contributor

Looked into #6683 , it's actually not a bug (or caused by my changes). It's working as designed - perhaps a bit unexpected for very small FOVs.

Contributor

lhofhansl commented Nov 30, 2017

Looked into #6683 , it's actually not a bug (or caused by my changes). It's working as designed - perhaps a bit unexpected for very small FOVs.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 30, 2017

Member

I'm not qualified to do a complete review and approve, some of this is beyond me.

Member

paramat commented Nov 30, 2017

I'm not qualified to do a complete review and approve, some of this is beyond me.

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Dec 3, 2017

Contributor

I split this form #6721 .

#6721 , #6723 and this one work best together.

Contributor

lhofhansl commented Dec 3, 2017

I split this form #6721 .

#6721 , #6723 and this one work best together.

@lhofhansl lhofhansl changed the title from Improve active object behavior and performance. to Improve active object behavior - extend active object in view direction. Dec 3, 2017

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 3, 2017

Member

I approve the concept (not an official approval) of:

Extend the range in the players direction in which active objects stepped at the server.

Don't feel qualified to judge the skipping of rounds.

Member

paramat commented Dec 3, 2017

I approve the concept (not an official approval) of:

Extend the range in the players direction in which active objects stepped at the server.

Don't feel qualified to judge the skipping of rounds.

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Dec 3, 2017

Contributor

@paramat That part is in #6721 now.

For games with complex mobs (like NSSM) it makes the difference between playable and not playable. In 90% of the cases there's no difference anyway.

I've have all these three on, and (apart from what @Fixer-007 notices, and I have since fixed) and seen no issues, other than the jitter goes away and there's more server time for generating the world.

Contributor

lhofhansl commented Dec 3, 2017

@paramat That part is in #6721 now.

For games with complex mobs (like NSSM) it makes the difference between playable and not playable. In 90% of the cases there's no difference anyway.

I've have all these three on, and (apart from what @Fixer-007 notices, and I have since fixed) and seen no issues, other than the jitter goes away and there's more server time for generating the world.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 3, 2017

Member

Aha ok, i'll try reviewing this one.

Member

paramat commented Dec 3, 2017

Aha ok, i'll try reviewing this one.

const s16 r_nodes = r * BS * MAP_BLOCKSIZE;
for (p.X = p0.X - r; p.X <= p0.X+r; p.X++)
for (p.Y = p0.Y - r; p.Y <= p0.Y+r; p.Y++)
for (p.Z = p0.Z - r; p.Z <= p0.Z+r; p.Z++) {

This comment has been minimized.

@paramat

paramat Dec 3, 2017

Member

The 3 lines above: spaces around '+'

@paramat

paramat Dec 3, 2017

Member

The 3 lines above: spaces around '+'

}
/*
Update list of active blocks, collecting changes
*/
// use active_object_send_range_blocks since that is max distance
// for active objects sent the client anyway

This comment has been minimized.

@paramat

paramat Dec 3, 2017

Member

'sent to'

@paramat

paramat Dec 3, 2017

Member

'sent to'

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 3, 2017

Member

I'm 👍 with everything up to the 'overload skip' stuff, which is where i'm struggling to completely understand. So i'd like a third dev to look at this.
Half an approval.

Member

paramat commented Dec 3, 2017

I'm 👍 with everything up to the 'overload skip' stuff, which is where i'm struggling to completely understand. So i'd like a third dev to look at this.
Half an approval.

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Dec 3, 2017

Contributor

Lemme update the patch here. I split off stuff from this PR, but neglected to update this one :(

Contributor

lhofhansl commented Dec 3, 2017

Lemme update the patch here. I split off stuff from this PR, but neglected to update this one :(

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Dec 3, 2017

Contributor

@paramat Updated now.

Contributor

lhofhansl commented Dec 3, 2017

@paramat Updated now.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 3, 2017

Member

👍

Member

paramat commented Dec 3, 2017

👍

@paramat paramat added the One approval label Dec 3, 2017

@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl

lhofhansl Dec 4, 2017

Contributor

Great. Thanks. Will merge in a few.

Contributor

lhofhansl commented Dec 4, 2017

Great. Thanks. Will merge in a few.

lhofhansl added a commit that referenced this pull request Dec 4, 2017

Optionally extend the active object in a players camera direction.
See #6667

By setting active_object_send_range_blocks > active_block_range a server admin
can allow clients to retrieve active objects futher out from the player at
relatively low cost to the server
(only objects in the players' view cone are considered).
@lhofhansl

This comment has been minimized.

Show comment
Hide comment
@lhofhansl
Contributor

lhofhansl commented Dec 4, 2017

@lhofhansl lhofhansl closed this Dec 4, 2017

t0ny2 pushed a commit to t0ny2/minetest that referenced this pull request Jan 23, 2018

Optionally extend the active object in a players camera direction.
See minetest#6667

By setting active_object_send_range_blocks > active_block_range a server admin
can allow clients to retrieve active objects futher out from the player at
relatively low cost to the server
(only objects in the players' view cone are considered).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment