-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
perf: redesign of area of interest #705
Conversation
5bd5ffa
to
a7e70a6
Compare
get | ||
{ | ||
if (ServerObjectManager == null) | ||
return Enumerable.Empty<INetworkPlayer>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is ServerObjectManager or interestManager being null an allowed state? should we be throwing here instead of returning empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this happens on testing. But to your question: yes it is a valid state, I can create an object with an NI, and then I can call Observers without spawning it. In my mind in that state, we just don't have observers.
/// Implement this class to provide a interest management policy and assign | ||
/// it to the <see cref="ServerObjectManager"/> | ||
/// </summary> | ||
public abstract class InterestManager : MonoBehaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to have something like this, so that the monobehaviour part is separated from the InterestManager part
public abstract class InterestManagerCreator : MonoBehaviour
{
public abstract InterestManager CreateInterestManager();
}
public abstract class InterestManager
{
// .. AOI methods here
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what exactly is the point? now to create an interest manager you will have to extend 2 classes where one of them just creates the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It doesn't need to be a
MonoBehaviour
, therefore it shouldn't be one - Later when we move to tick based system
NetworkServer
(or something else) will be calling Tick/Update onInterestManager
. Having it as aMonoBehaviour
may lead people to use Unity's callback when they shouldn't be used. - it not being a
Monobehaviour
makes it easier to support .net core (minor issue)
} | ||
|
||
internal void ServerUpdate() | ||
{ | ||
if (observers.Count > 0) | ||
IEnumerable<INetworkPlayer> observers = Observers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can inline this variable
@@ -1242,7 +1047,9 @@ internal void SendToObservers<T>(T msg, bool includeOwner = true, int channelId | |||
{ | |||
if (logger.LogEnabled()) logger.Log("Server.SendToObservers id:" + typeof(T)); | |||
|
|||
if (observers.Count == 0) | |||
IEnumerable<INetworkPlayer> observers = Observers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can inline this variable
080142b
to
7db7b1f
Compare
The AOI system we inherited from Mirror is poorly designed. It's API requires O(n^2) algorithms, it is highly complex, and it is not flexible enough. This PR implements AOI in a completely different way. The NetworkIdentity objects no longer track visibility, and neither do NetworkPlayer. When the server spawns an object, all it does it raise the Spawned event, the AOI system will subscribe to this event, and then show that object to all the relevant players. Likewise, when a player joins, the AOI will listen for the event and show the relevant objects to the newly created player. In this fashion, we decouple AOI policy from NetworkIdentity and NetworkPlayers. The AOI system can keep the relationship between them in any way that is suitable. In this PR, I implement the simplest InterestManager called GlobalInterestManager. This Interest Manager will just show all objects to all players. Implement the class InterestManager to create any policy desired. I expect to create a Spatial Hashing interest Manager BREAKING CHANGE: Removed NetworkVisibility, extend InterestManager and attach to the ServerObjectManager instead.
Kudos, SonarCloud Quality Gate passed! |
Closing old PR New AOI branch here #948 |
The AOI system we inherited from HLAPI and Mirror is poorly designed.
Its API requires O(n^2) algorithms, it is highly complex, and it is not flexible enough.
This PR implements AOI in a completely different way.
The NetworkIdentity and NetworkPlayer objects no longer track what they can see.
When the server spawns an object, all it does is raise the Spawned event,
the AOI system will subscribe to this event, and then show that object to all the relevant players.
Likewise, when a player joins, the AOI will listen for the event and show the relevant objects to the
newly created player.
In this fashion, we decouple AOI policy from NetworkIdentity and NetworkPlayers. The AOI system can keep the
relationship between them in a suitable structure.
In this PR, I implement the simplest InterestManager called GlobalInterestManager.
This Interest Manager will show all objects to all players.
Implement the class InterestManager to create any policy desired. I expect to create a Spatial Hashing interest Manager in the near future
BREAKING CHANGE: Removed NetworkVisibility, extend InterestManager and attach to the ServerObjectManager instead.