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

Implement client bubble PVS. #983

Merged
merged 5 commits into from Feb 23, 2020
Merged

Conversation

@Tyler-IN
Copy link
Contributor

Tyler-IN commented Feb 16, 2020

You guys call it bubbling. PVS means Potentially Visible Set.

@Tyler-IN Tyler-IN changed the title Implement client bubbling. Implement client bubble PVS. Feb 18, 2020
Copy link

Clyybber left a comment

Awesome

@Tyler-IN

This comment has been minimized.

Copy link
Contributor Author

Tyler-IN commented Feb 19, 2020

FYI there are still some outstanding known errata w/ picking parents/children.

Needs more testing to narrow down when/where/how.

I will be making client changes to support position being NaN in state updates as "don't vanish until seen" mechanics so the client can't see things are being unseen.

@Tyler-IN Tyler-IN force-pushed the ImpromptuNinjas:bubble branch 4 times, most recently from 58c4c65 to 9827f0a Feb 20, 2020
@TYoung86

This comment has been minimized.

Copy link

TYoung86 commented Feb 22, 2020

from discord, a rough explanation;

so when a player joins, they get sent the entire world
the last seen dictionary stores anything that they've seen that has changed since that tick
as just a (uid,tick) pair
if they come across something that has a different change tick than they've last seen that's inside their bubble, they get the state
if it hasn't changed since they last saw it, no state is sent nor needs to be
if something changes outside their bubble that they have previously seen, the NaN covers that situation that would produce a ghost
the client can be updated (imo separate PR) to handle NaNs by hiding the object temporarily until the next position comes in, instead of updating the pos
or the process can be changed to just send some new "unsee this" message instead of position NaN
when they "unsee" something, it's removed from the last seen dictionary
so it doesn't blow up and replicate the world inside the dictionary
the lastSeen dictionary only gets queried or built based on their bubble
so it should virtually always be a subset of the world, worst case they've somehow seen everything and it's the full set of uids but just ticks
in a future PR I'd like to exclude sending container contents, and maybe re-work some of the handling so sending updates to parents can be more relaxed
right now immobile objects like snap grid objects outside the bubble get NaN'd prematurely when they're destroyed if they're in some common parent/child heirarchy

Tyler-IN added 2 commits Feb 16, 2020
include contained entities

fix line edit crash when clipboard contents are null somehow

use CVar for update bubble range

remove seenMovers that get NaN'd out of MaxUpdateRange - huge reduction in network traffic from bullets

cut default of MaxUpdateRange down to 12.5 for now

maybe handle teleported things

handle removing player state on disconnect

fixed positional audio errors

make UpdateEntityTree also update player's last seen ents

make net.maxupdaterange cvar tunable at runtime w/o impacting performance

back to position nan as means to hide from client

revert work pooling as the wrong player was getting the wrong game state

ffs needed to think less about *if* something is *seen* and more about *when* it must be *unseen*

clean up usings

handle parenting and sequencing issues that arise on the client

gather needed ents to update recursively

applied suggestions from code review

fix missing recursively contained ents

make assumption that client last saw things on first tick instead of tick zero, as zero would be the "from tick" and first would be he "to tick"

add First tick as constant to GameTick due to relevance above

renamed includedEnts to checkedEnts to make usage more clear

added comments

inverted some conditions to flatten parts of the pvs logic

fixed sending states when already seen (lastChange < lastSeen to lastChange <= lastSeen)

fix sending states when no actual changes (other bugs?)

local caching of currentTick, remove priorTick
…ating network state if AttachParent isn't invoked
@Tyler-IN Tyler-IN force-pushed the ImpromptuNinjas:bubble branch from 9827f0a to 98dab7d Feb 22, 2020
Copy link
Member

PJB3005 left a comment

Would it be possible to introduce an explicit off switch (that also would work if changed mid game?).

if (!source.SetPosition(entity.Transform.WorldPosition))
{
Logger.Warning("Can't play positional audio, can't set position.");
return null;

This comment has been minimized.

Copy link
@PJB3005

PJB3005 Feb 22, 2020

Member

Not sure it's a good idea for this to ever return null. Even in the worst scenario it might just be better to have this return a dummy value that's seen as stopped?

This comment has been minimized.

Copy link
@Tyler-IN

Tyler-IN Feb 23, 2020

Author Contributor

The only outer caller doesn't evaluate the result of Play.

How should it handle this case?

if (!source.SetPosition(coordinates.ToMapPos(_mapManager)))
{
Logger.Warning("Can't play positional audio, can't set position.");
return null;

This comment has been minimized.

Copy link
@PJB3005

PJB3005 Feb 22, 2020

Member

Ditto on returning null here.

This comment has been minimized.

Copy link
@Tyler-IN

Tyler-IN Feb 23, 2020

Author Contributor

PlayAudioPositionalHandler as the only outer caller doesn't check the return value.
Do you want it to verify if a sound played?

{
_checkDisposed();

var (x, y) = position;

if (!ValidatePosition(x, y))

This comment has been minimized.

Copy link
@PJB3005

PJB3005 Feb 22, 2020

Member

This feels like really bad API design to me.

Maybe make this throw on NaN and have the uses explicitly check for it before calling?

This comment has been minimized.

Copy link
@Tyler-IN

Tyler-IN Feb 23, 2020

Author Contributor

Outside of the API it's used like this;

if (!renderer.Source.SetPosition(renderer.TrackingEntity.Transform.WorldPosition))
{
  Shared.Log.Logger.Warning("Interrupting positional audio, can't set position.");
  renderer.Source.StopPlaying();
}

Adding [MustUseReturnValue] makes the API require the bool to be checked or issue a warning on complying IDEs.
The caller should just stop playing the sound if the position is NaN, because that indicates it's an entity the client is unaware of the position. If this ever occurs, it's probably intentional.
I think Audio that's not relatively quiet or localized needs to be sent with a specifically with a position and not an entity. There should be no guarantee that a sound can play at any position.

This comment has been minimized.

Copy link
@Tyler-IN

Tyler-IN Feb 23, 2020

Author Contributor

Do you think it should be TrySetPosition instead?

#pragma warning restore 649

private int _nextServerEntityUid = (int)EntityUid.FirstUid;
private float? _maxUpdateRangeCache;

This comment has been minimized.

Copy link
@PJB3005

PJB3005 Feb 22, 2020

Member

This thing doesn't actually get assigned outside of that null down there.

This comment has been minimized.

Copy link
@Tyler-IN

Tyler-IN Feb 23, 2020

Author Contributor

That's correct... so the server can update _maxUpdateRangeCache related cvar once per frame.

This comment has been minimized.

Copy link
@Tyler-IN

Tyler-IN Feb 23, 2020

Author Contributor

Oh I read this wrong, I thought you meant it doesn't get assigned except for the null and where it gets assigned...

        public float MaxUpdateRange => _maxUpdateRangeCache
            ??= _configurationManager.GetCVar<float>("net.maxupdaterange");

^ It gets assigned there. It's just 2 lines down.

This comment has been minimized.

Copy link
@PJB3005

PJB3005 Feb 23, 2020

Member

Oh, my bad.

Robust.Server/GameObjects/ServerEntityManager.cs Outdated Show resolved Hide resolved
public List<EntityState> UpdatePlayerSeenEntityStates(GameTick fromTick, IPlayerSession player, float range)
{
var playerEnt = player.AttachedEntity;
if (playerEnt == null)

This comment has been minimized.

Copy link
@PJB3005

PJB3005 Feb 22, 2020

Member

Players in the lobby have no entity, so...

This comment has been minimized.

Copy link
@Tyler-IN

Tyler-IN Feb 23, 2020

Author Contributor

What does this mean?

Tyler-IN added 3 commits Feb 23, 2020
…nt's Children collection and other stuff

rotating parent entities now are visible with their orbiting children if one of their children moves into view
introduce net.pvs to configure enabling/disabling PVS
@Tyler-IN

This comment has been minimized.

Copy link
Contributor Author

Tyler-IN commented Feb 23, 2020

Would it be possible to introduce an explicit off switch (that also would work if changed mid game?).

Added.

@Acruid
Acruid approved these changes Feb 23, 2020
Copy link
Contributor

Acruid left a comment

There are a few edge cases that need to be looked at, mainly around world parenting and shuttles. As far as I can tell these problems are because the TransformComponent is poorly implemented, and really needs some testing and API tweaks. The main features were successfully implemented and seem stable enough for the CM test at the end of the month, so I am approving this.

@PJB3005 PJB3005 merged commit 214f9f0 into space-wizards:master Feb 23, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.