minolin / acplugins

R&D about the upcoming server plugin infrastrucure of Assetto Corsa, driving simulator by Kunos simulazioni
Apache License 2.0
18 stars 4 forks source link

Blank out disconnected driver name/guid/team in MsgCarInfo? #21

Open minolin opened 9 years ago

minolin commented 9 years ago

I don't like the message behaviour a MsgCarInfo will report it's last driver when he is disconnected. Currently my plugin code handles this, but I'm thinking about moving that to the AcServerPlugin base.

Especially people who aren't that familiar with the acServers way to work would expect an empty driver seat, instead of the last one that was connected. What do you think?

flitzi commented 9 years ago

Yes, the stuff with the fixed and reused car slots messes quite a few things up (e.g. Session Results). That's why I implemented some more advanced stuff in my ReportPlugin to keep track of the all the drivers which connected within a session and which car is currently used by which driver. To be honest I did not like your implementation with the dictionary which simply caches MsgCarInfo very much. That was the main reason why I created the bare AcServerPluginBase class, from which the ReportPlugin derives. But I too thought about adding stuff to the PluginManager to provide easy access to information and perhaps even some functionality for the plugins to use. What do you think about adding some of the SessionReport functionality to the PluginManager: https://github.com/flitzi/AC_SERVER_APPS/blob/master/AC_SessionReportPlugin/ReportPlugin.cs

minolin commented 9 years ago

Yes, definitifly. But why isn't it derived from the AcServerPlugin class? You hide every functionality we introduce there.I would turn it the other way round and create a Subclass like CleverAcPluginManagerA, from which the plugin can derive. This could be of course something we merge into the ReportPlugin (that isn't available on my side of the fork :D)

Easily accesible information in caches do belong into the base classes, or at least in a abstract subclass.Don't you like the dictionary implementation of the CarInfo, or the fact that it's in the AcServerPlugin class?

Currently I'm hunting for

flitzi commented 9 years ago

OK, cool. I will work something out and create a pull request when the first version is done.

minolin commented 9 years ago

I'm currently trying as well, what exactly did you plan?

Regarding different AcServerPlugin classes, we should define them inside the acPlugins4net assembly, and unseal the AcServerPlugin "protected internal sealed override"s to "internal override", so they can be used within the MinosRealtimePlugin or FlitzisEvenMoreFancyRealtimePlugin, but not by the final plugins.

flitzi commented 9 years ago

Regarding deriving from PluginManager: Don't like this because this would mean that Plugins that use different SubClasses of the PluginManager would no longer be compatible. Therefor I will add the stuff to the PluginManager itself, but in a way that Plugins are not affected if they don't want to use any of the additional functionality of the PluginManager.

minolin commented 9 years ago

Mh k, I didn't thing about the manager itself, but yes. Would be also efficient to do all the stuff only 1x, instead of 1x per plugin.

minolin commented 9 years ago

Your PluginManager structure is pretty good. I'm lazy, so I like elegant things :+1: Just checked in a very dirty Realtime monitor as Internal plugin (branch), that does expose an DistanceDriven event. I'm a little bit worried about synchronizing all n CarUpdates (I hope they come in one bulk) for position comparison between cars. You can't compare 2 car updates with a second difference; this is easily 50-100m on the track :-/

minolin commented 9 years ago

YAY. They come in within 1-2ms, and in order by client_id

flitzi commented 9 years ago

Ok, I've pushed my solution, I hope you like it so far, I hope we can create a good plugin base on this together. I've seen you have a new RealTimeMonitor. I think with the implementation of the DriverInfo at manager.CurrentSession this is redundant, but if you are missing something now, please let me know and I will try to add it. Regarding distance, good info, maybe we should trigger our CarUpdate message only after we got updates from all cars then. Will think about this.

I'm using some new objects to store the session info in a user friendly way. Some are close to the message objects you create from the udp packets, some a bit more advanced. I wonder if we should directly pass those new objects in the OnLapCompleted, OnCollision, etc. methods instead of the original messages. I for sure think it would make sense, but it would mean some refactoring on all plugin implementations, so I haven't done it yet because I'm not sure if you will like it. Maybe a task for tomorrow if you agree on it. What do you think about also moving the collision bag to the manager? I personally think that every collision should count no matter if it wasn't the initial contact, but nevertheless, I like the idea to group the collisions. Maybe another task for tomorrow.

minolin commented 9 years ago

Didn't understand it all after a first glance, but yes, looks good. The DriverInfo is/will be absolutely required. Of course we can "ask" for MsgCarInfo and MsgCarUpdates all the time, but this is just not necessary. Passing them through the event handlers is a good idea, maybe as a second parameter to everything that is car related.

I will try (lol really) to merge this pull into the master + my branch and try to add my functionality. We worked perfectly on different edges of the project; I dodged the DriverInfo and framwork stuff you did, but researched around meters, spline pos and so on. Of course my Realtime Monitor is completly redundant, but I felt it's good if we try different things and merge what we like.

Regarding distance, good info, maybe we should trigger our CarUpdate message only after we got updates from all cars then. Will think about this.

If we knew what the biggest carId will be (maybe because we rely on ClientLoaded + ClosedSession) we could trigger the calculation just after this one. Not that failsafe, maybe it would be better to use a thread with +2 or 3ms to check if something more came in and do it then. The timing when we calculate won't be that important as long as the data is created from a very thin timeline. Think about the possibilities, we could use a cheap OrderBy-SplinePos to estimate how near the cars are and even would see overtakes, be it just an attack or a succesful one. Combined with a base "per meter" this could get really helpful, not only for the MR.

minolin commented 9 years ago

Good work, had to laugh really often, my todo-list is getting smaller every pull request :D You even implemented the admin-command, yay!

Edit: NVM the following, now I found the PluginManager.CurrentSession.Drivers which is exactly what I'm looking for. Still we question why we have the ConcurrentDict?

A question: Why is the carUsedByDictionary a ConcurrentDict? Do I miss something or is this just a relict of my CarInfo dict? AFAIK we process messages stricly singlethreaded. The way my plugin would use it looks very inefficient, I'd prefer just extracting the .Values (I can do that, but I have the feeling that I'm missing something)

flitzi commented 9 years ago

Glad that you like it.

Good idea with the bulk car update. I've made a small adjustment on how/when the OnBulkCarUpdate is triggered. It should be a bit more failsafe.

I have an idea how to determine the track length, but I probably won’t get to implement it until this evening.

The carUsedByDictionary is a normal dictionary. I added some locking to the PluginManager because the CommandEntered (Console) method might be called from another thread.

Regarding MR ratio: As I said in one of my PMs, you can drive quite some distance cleanly without ever completing a lap, but on the other hand every incident is counted, even if the lap isn’t completed. So I think using the actual driven distance would give a much more accurate rating than only counting the completed laps. I don’t know how much effort changing this on the backend will be, but to me this sounds like a good solution. Nevertheless I will store the actual driven distance per lap on the LapInfo object. Comparing how far someone drives per lap could be an interesting topic, and maybe even determine if some is cutting the track, if his lap distance is shorter by some margin, although I’m not sure if this will be accurate enough for this kind of stuff. Regarding the OnLapCompleted, OnCollision etc methods and the passed objects, I think we could implement some overloading methods, so the dev can chose which one he wants to use.

minolin commented 9 years ago

ty for the update.

Regarding MR ratio: As I said in one of my PMs, you can drive quite some distance cleanly without ever completing a lap, but on the other hand every incident is counted, even if the lap isn’t completed.

Not completed laps are a minor issue, compared with the problems inside the first lap. It's very difficult to work with a ratio of incidients/base when you have to guess one for the first lap. 1 corner? trackcorners/2? Your incidients/corner go nuts all the time, and you even have to consider this could be a nords-endurance you want to kick somebody based of which progress?

So my new plan is to use&send the first 100 / 500 /1000m driven as event, then every Xm. Of course the current meters are also sent with the collision stuff, so I can always say what progress we are talking about. Removes a ton of problems and complexity. This is one of the reasons I wanted to abandon the beta: getting time to build this and change the wcf contracts accordingly without having to maintain 39 servers out there.

flitzi commented 9 years ago

T1 pileups are a problem of course, but no one should ever be kicked automatically because of multiple contacts in T1, the chance of false positives is too big. I think short term consequences of T1 pileups are only solvable with an admin in place to analyze the situation (hopefully we get the option for viewing replays during MP sessions soon).

Are you giving everybody a starting MR of C or B, for example 500km with 20 incidents, something in the middle of the distribution, and then let them drift away in whatever direction? You could even do this on a smaller scale for a live session locally, for example start with 50km and 2 incidents, and if they reach a certain local MR of D and then W they first get a warning and then be kicked. Not banned, just an explicit sign that this is not tolerated (unless of course his global MR became too low because of this, which actually is a ban then). The backend only needs to be contacted once per session to update the MR, and of course determines who is allowed to join and who not, saves bandwidth for the acServer, which is very heavy already. And regarding how the incidents are counted, I still think that everybody involved should get an incident, but of course only once per bag, so you don't get multiple incidents in very short time. I would maybe even change the bag to be a bag per driver, or in other words, a driver can only get one incident every x seconds, for example every 5 seconds. I think which other car(s) is/are involved in the incident plays a minor role. The only thing that is important is "how often do you get incidents". I wouldn't even make any difference if "got rear ended" or "was overtaking a lapped car", incident is incident. But you are probably already much further along, so you know what is working and what not. Someday I would really like to take a look at the data you are collecting, maybe after my holidays, if you allow it.