[SVN:r3254] LoS Check Manager

[Leodagan]

As I was working on a new implementation of Standard Mob Brain I realized I needed some LoS Check, and prevent them from doing too much query and try to cache results to reduce the LoS Checks Frequency...

I nearly finished a LosCheckMgr, that can be instanced in Region Object (LoS check have no way to be across Regions...) and be used to throttle Los Check, store them in a cache, register Event Handler to an unfinished LoS Check (thus making One check available for multiple object wanting the results) and other tools...

This Manager can make Blocked Los Check (will wait for the response event before returning bool result), Non blocked Los Check that needs a IDOLEventHandler Object for returning results

Non Blocked Checks can get results from a previous Blocked query that will dispatch Notify message to registered IDOLEventHandler, cache can be enable or disable Timeouts can be default const (will be made serverproperties) or defined in the Method Call (Timeout for Blocked is the max waiting time, for Non-Blocked it's the cache validity)

Code parts like

Code: Select all

Player.Out.SendCheckLos()

will need to be replaced by

Code: Select all

Player.CurrentRegion.LosCheckManager.[Blocking]LosCheck(checker, source, target, [IDOLEventHandler])

The Call back part will need an implementation in the "Notify();" Method of a IDOLEventHandler Object (mostly all object can inherit this Interface Safely)

The Blocking Checker are for testing purpose for now, they could be use in a Threaded Environment (If Mobs were to be lightly Threaded, this kind of scheduling won't be bothering the RegionTimer and will make it easier to take decision when Los Check is really made !)

I'm not posting files because it's not tested, If it's not wished to have this kind of manager in DOLCore I'll need to fork my Code for future Commits.

[Leodagan]

Ok my firsts Tests are good...

It's not tested in heavy environment and could lack debugging for now

Requesting Comments if anyone want to read this.

Edit : BlockingLosCheck are bugged for now (sometime there isn't a finish LoSCheck event if it's the cache of a non-blocking Check)

[Tolakram]

Did not have time to look, but I like the idea.

With any client communication you will need to expire a check and not block, otherwise both dropped packets and purposeful "hacking" can cause all kinds of issues. Generally you wait for a reasonable time for a response and if no response happens you fail. This is why both with DOL and Live you will occasionally get LOS errors on laggy connections.

[Leodagan]

It's kind of implemented.

I use the existing PacketLib to do that, so it's still "callback" driven.

My blocking LoS Check ares functions that create an event thread and send the LosCheck, then wait for the event to trigger the thread signal and return the answer or exits on timeout (timeout exceptions should be "catch" to do something when it doesn't work).

The non-blocking checks are here for the easiest "drop-in" replacement in current classes that used callbacks.

And Blocking LosCheck with default timeout around 400/1000ms should NEVER be used in a class working on a shared timer (like RegionTimer)

The best wait to use them would be when we are in a dedicated thread that doesn't need Real Time, or in a Threaded Tasks that could "join()" later on...

[Leodagan]

Ok Blocking Los Checker are somewhat not working at all

The event waiting proc seems to block all other timer (I think timer are shared in spellhandler or similar)

So it always timeout before it can handle the answer...

I'm beginning to think this have no use at all (it should be used outside shared timer, and I'm not even sure debugging could make this works properly), I'll remove them to keep an Event based Check only.

The event based checks are the simplest to use in place of the old callbacks found everywhere in code !

[Leodagan]

Here is the "non-blocking" version of the Los Manager.

It's tested to work in simple situation for now, I'm posting it for comments

I began to change Los Checks in spellhandler, it's going ok, I tried to do the same in GameNPC, it's a bit tough !

This Los Manager is able to handle NPC to NPC checks using a player in the vincinity so much of the code used elsewhere is useless

And Attacking + Casting code in there should definitely be in Brains, the Notifications are gonna be hard dispatch from aggroing checks, attacking checks, casting checks as they all notify the same object (GameNPC changed implementing IDOLEventHandler)

[Leodagan]

Notification is not the best way for some really dependent check Los Path...

It's hard to replace the current system, in spellhandler if there is multiple LoS check it's a pain to keep all the details about the current Object and what he's doing, to continue in the Notify when LoS is received, LoS could be receive for any reason, not knowing if I must continue...

I tried to make Cache checking before going into event handler, but it doesn't give the results I expect !

I'm making a delegate method for LoS checks to purely mimic the old behavior, but at least we they are in the same place as all other Checks

[Leodagan]

An update that can handle delegate.

there is a default event handler that can take a delegate to keep my code as is

Tested with spellhandler and gamenpc Los Check.

Going pretty well, cache works, pending queue rarely miss it's result

I got once two packet at the same time with OK and KO, I don't know why, the LoS should have been good, and the second packet could not find its pending data, it just returns a warning and didn't notified registered objects, maybe it needs more locking, anyway I think the KO was not a good response.

Not much exceptions even if they should be handled in some way

With Throttling parameters I reject Los Check at some times, I should try to queue them...

I'll begin to update most part of the code using this manager, I can log easily now that all checks are in one place.

[Graveen]

Good job Leo !

[Dinberg]

I like this change, but can we retain support for the old SendCheckLos() syntax aswell, and mark it as depreciated? There's alot of custom scripts that will be broken by the change.

I like the new syntax best because if DoL ever implements server-side LoS checks it will clearly be better suited

[Leodagan]

Indeed Dinberg, having all los check managed by a class can make it easy to change the behavior of all los checking, serverside los would just need some overrides to make it work !

Good idea for the obsolete call, I was going to update PacketLib today to prevent some LoS checks overwriting, now I think I'll make a copy of the old PacketLib implementation into a new one that use my callbacks so both can work, and the previous one will warn for deprecation

--------
Apart from this

Some test since yesterday on my server

There are many pending cache miss, I think there are multiple LoS query sent without being added to pending cache, or got over the timeout and made a new los query receiving 2 responses (with lag) and having only one pending data (which is overwritten every new checks).

The default LoSCheck callback only returns the sender and the target object, I need the source object too as I'm using third-party los check for most of PvE and even "EvE" (and unavailable players for LoS Checking)

The pending data key "GamePlayer Checker, ushort object" is really a pain with many players sending third-party checks !

I'm going to provide a new LosCheck method for PacketLib that takes a callback handling Source ObjectID, and maybe make something like a queue to LoSCheckMgr but after some testing with the new callbacks (handling queue, third-party, timeout, cache, is gonna make this class hard to read...)

Got to take the chance of having some players on the server that generate a lot of debug data

[Graveen]

Thank you Leo. For now i suggest a SP based change (old vs new), and the start point of the manager in IsAllowedToAttack(), IsAllowedToCast() methods.

Basically, pets got their own implementation because this is something important, such as guards, players etc, but all will benefit a real manager.

If all is ok, we could simply fine tune it (for external , rvr areas only, dungeons, complete world...)(for guards, pets, players, factions...) from the server rules (or the manager itself, but at one moment, the behaviour between PVP/RVR/Coop would be different, here is what i think serverrules is a suitable place)

Good job !

[Leodagan]

What do you mean by a SP based change ?

For now I wrote a copy of SendCheckLos() in IPacketLib that take another callback "Type" (CheckLOSMgrResponse instead of CheckLOSResponse)

They both have the same code except for the callback type and the TempProperties Key used (Key changed from "LOS+data" to "LOSMGR+data") to store the callback (I just realized that LoS checking was doing that...) just going apart from the initial LoS code could help me with handling them and let other code in game use the old implementation...

Here are the patch for this : (I think it won't change any existing code)

[Leodagan]

Updated all LosCheckManager based on the new callback with finally the source Object ID ! (the old one must have been written when LOS was only used to check Player to Target)

This has simplified most of my code, pending data is mostly useless now and it's just a timestamp...

Los Check coming out without any pending data don't bother me I just store them to cache... (this could be used by a modified client to Poison Los Cache, it should be easy to make an exit condition if the incoming LoS results weren't requested)

Unwanted Los Check will be able to update LoS cache, but won't trigger any notify to event handler (Event registering is somewhat tied to pending cache)

Third-party check now get its data from cache before finding a player to check, if a cached query is made (it still needs to send an arbitrary player as mosts of Event handlers checks that LoS events are coming from a Player, I'm thinking of making it nullable or use another object sender)

Added some more tools to default cache timeout in case of PvP, PvE or EvE (Environment vs Environment will be useful for Faction mobs fighting each others...)

[Leodagan]

The [Obsolete()] Attribute should be in IPacketHandler Interface or it doesn't report at compile time

By the way putting this Attribute near a method is a GREAT way to find all object using it

I can now finish my update to Core parts where there was still los check (this won't be published, it Breaks a lot of thing, but it's my best way to test in Live !)