Starlight [C#/.NET Core/Dapper]

[Damien]

Hello,

Today I introduce you to Starlight, a complete rewrite of Alias (an emulator I've been working on for a while). The old source was messy due to lack of experience and others brining in their own ideas, but alot of the old source code will be recycled, making the time spent on it not a waste.

The main purpose of the Emulator is for my own leaning purposes, which is why I'm making a dev thread (all criticism approved). The project is in it's very early stages so there isn't much to show, but I'll try to provide some snippets bellow aswell as link the git repository when there is one. But anyway here's what you can currently expect:

Starlight

Features:

• Using C# 8.0
• Using .NET Core 3.1
• Using Dapper
• Using DotNetty
• Using DependencyInjection
• Plugin system

Game Features:

• Handshake (Authentication)
• Hotel View

Snippets:

Spoiler

Spoiler

Screens: (not much)

Spoiler

Credits:

• SpeedBlood: for helping with the core and helping me when needed
• Arcturus: for being the best reference point for packet structures
• Other Emulators: for more referencing because I'm dumb (sometimes)

 

[Liam]

Thread approved. Good luck with development!

Nice to see you around again, btw. Always been a fan of your work.

 

[Joe]

Good luck Damien! You’ve had some great projects in the past, nice to see some active development in this section.

 

[Adil]

Dapper? Very nice (or should I say, dapper)
Good luck!

 

[TheGeneral]

Its best practice to use named arguments in logging. Reason for this is so that string interpolation isn't evaluated at runtime when logging is disabled, ignored or discarded. Also using named arguments you could use other analytics tools for further post processing.

 

[Damien]

Its best practice to use named arguments in logging. Reason for this is so that string interpolation isn't evaluated at runtime when logging is disabled, ignored or discarded. Also using named arguments you could use other analytics tools for further post processing.

Hi, thanks for the feedback. String interpolation isn't something I've ever heard before but after doing some research I understand the point you're making.

My log messages have all been refactored now so string interpolation will no longer be an issue, thank you!

 

[TheGeneral]

If you want me to give more feedback, feel free to hit me up. Would be good to have more quality emulators around.

 

[Damien]

Small update, messenger is now working. Adding, removing, requests ext all work (aswell as when the target is offline) and I also took the time to update the message handler a little so it can parse in pre handled arguments. What this means is that the packet get gets processed before the event handles the data. Example bellow. This allows me to define variables for each event and reuse them without having to set them locally in the event handler, so the event handlers should look alot less cluttered now. I still have a few things left to do in the messenger like sending pms, room invites, stalking/in room status. But I'll finish that off when I've started the room engine.

EventArgs:

public class RemoveFriendArgs : IEventArgs
{
    public IList<uint> TargetIds { get; private set; }

    public RemoveFriendArgs()
    {
        TargetIds = new List<uint>();
    }

    public void Parse(IClientMessage message)
    {
        int count = message.ReadInt();
        for (int i = 0; i < count; i++)
        {
            TargetIds.Add(message.ReadUint());
        }
    }
}

RemoveFriendEvent handler:

protected override async Task HandleAsync(ISession session, RemoveFriendArgs args)
{
    foreach (uint targetId in args.TargetIds)
    {
        if (!_playerController.TryGetPlayer(targetId, out IPlayer targetPlayer))
        {
            IPlayerData targetPlayerData = await _playerController.GetPlayerDataByIdAsync(targetId);
            if (targetPlayerData == null) continue;

            targetPlayer = new Player(targetPlayerData, null);
        }

        if (!session.Player.MessengerComponent.FriendExists(targetPlayer.PlayerData.Id))
            continue;

        session.Player.MessengerComponent.RemoveFriend(targetPlayer.PlayerData.Id);
        await session.WriteAndFlushAsync(new MessengerUpdateFriendComposer(targetPlayer.PlayerData.Id));

        if (targetPlayer.Session != null && targetPlayer.MessengerComponent != null)
        {
            targetPlayer.MessengerComponent.RemoveFriend(session.Player.PlayerData.Id);
            await targetPlayer.Session.WriteAndFlushAsync(new MessengerUpdateFriendComposer(session.Player.PlayerData.Id));
        }

        await _messengerController.RemovePlayerFriendAsync(session.Player.PlayerData.Id, targetPlayer.PlayerData.Id);
    }
}

An image:

Spoiler

 

[exmma]

good luck with this!!!!!!!!!!!!!!!!!!!!!!!!!

 

[Gabrielle]

Just a small little thing, in later C# versions, it's possible to do using a different way which saves some lines.

using var connection = dbProvider.GetSqlConnection();

Good luck! The code looks good so far!

 

[Damien]

Just a small little thing, in later C# versions, it's possible to do using a different way which saves some lines.

using var connection = dbProvider.GetSqlConnection();

Good luck! The code looks good so far!

Does this provide any real benefit over using the older using statements?

I know they're less lines of code, but I prefer using them because then I know when the variable is getting disposed. If I had a further 50 lines of code for instance after the using statement the variables don't get disposed till those lines have executed. I could use both if there is any performance benefit to them, but I think I'll keep to using statements for everything so it's the same across the board.

 

[TheGeneral]

Does this provide any real benefit over using the older using statements?

I know they're less lines of code, but I prefer using them because then I know when the variable is getting disposed. If I had a further 50 lines of code for instance after the using statement the variables don't get disposed till those lines have executed. I could use both if there is any performance benefit to them, but I think I'll keep to using statements for everything so it's the same across the board.

Technically there is no difference. With a scoped using you've got a more control over when the object is disposed. With an non scoped using statement its at the end of the method. If thats supposed to be the lifetime of your disosable object then go with the new style.

Non scoped using statements also save on horizontal space as there is one less indentation per scoped using. Imaging having 3 scoped usings, it saves 12 characters (depending on your indentation settings) in width.

Post automatically merged: Nov 21, 2021

Also your message parsing could be simplified to something like:

TargetIds = Enumerable.Range(0, message.ReadUint()).Select(_ => message.ReadUint()).ToList();

Post automatically merged: Nov 21, 2021

Why are you using a dedicated Parse(IClientMessage) method while initializing the object from its constructor is pointless atm? How about passing the IClientMessage object to the constructor instead?

 

[Joopie]

Why are you using a dedicated Parse(IClientMessage) method while initializing the object from its constructor is pointless atm? How about passing the IClientMessage object to the constructor instead?

I perhaps can make an educated guess and reason why it's done that way, seeing SpeedBlood's name in the credits.

You can construct a new instance of a given class via a type parameter. One of the criteria's is to define a few generic type constraints, such as class and

You must be registered for see links

. Defining these type constraints forces you to create a parameterless constructor. Leaving the only way to fill the instance by a seperate parse method.

 

[TheGeneral]

Going generic in your interface doesnt mean you are required to have a parameterless constructor. Just do something like this:

typeof(MessageType).GetConstructor(new[] { typeof(IClientMessage)}).Invoke(new[]{message})

And wherever you register your messages you can check if that type has a constructor that accepts IClientMessage

The point of a constructor is so that the object is fully constructed and initialized, ready to be used. Having an extra function to be an hardrequirement to be called on that specific object to initialize it, is bad practice and defies the purpose of having a constructor. A better approach would be to have a static parse function that fills out the parameters values through the constructor.

 

[Joopie]

It seems I forgot the why. Yes I do agree with what you said. It can also cause weird side effects, when you need a specific method to be called first in order to do something with it.
However I find some use cases, for example this one, where such errors are minor. It's an object holding data and can be loaded in from a IClientMessage. It doesn't do any specific logic other than that, and it should contain more than that. The infrastructure is in place that handles it.
Then there's the question what kind of logic do you really want in a constructor? Without cluttering it too much you basically only ever do the following: initialize fields and/or properties. Constructors that can cause exceptions isn't one of them (you're doing IO operations).
Another valid point is performance. This type of class gets constructed ever time you receive a packet. I find in this case performance a valid argument. The best use would be to just call the constructor directly. Second best option would be to call the parameterless constructor via Activator. The worst case scenario would be to call Type.GetConstructor. At compile time the generic new constraint translates to the Activator. Another unexplored solution might be to use expressions, but not sure if that would be simpler/understandable.
Other benefits of using the such construction would be the (pre-)compile time checks the IDE and/or compiler gives you. You're also avoiding exceptions at runtime, because you're guaranteed to have a parameterless constructor.
Your solution wouldn't work because you still need to know the specific type on which you want to call the static method. Defeating the purpose of a generic implementation. Another drawback is testability. Static methods are a pain in the ass to test and should be avoid. Static methods are hardly ever the solutions.
Solution to avoid this type of initialing is to use a dedicated parser class that specifically constructs that data structure. Does the extra class, and thus complexity really make it better tho?
I agree such method containing side effects should be avoid, but in this case it's short of justified.

 

[TheGeneral]

However I find some use cases, for example this one, where such errors are minor. It's an object holding data and can be loaded in from a IClientMessage. It doesn't do any specific logic other than that, and it should contain more than that. The infrastructure is in place that handles it.

Great, then make it a struct.

Then there's the question what kind of logic do you really want in a constructor? Without cluttering it too much you basically only ever do the following: initialize fields and/or properties. Constructors that can cause exceptions isn't one of them (you're doing IO operations).

Actually, no. The data has already been received (At least I really hope). As the message has a prefixed length + header thus I assume that IClientMessage contains all the bytestream bytes already.

Another valid point is performance. This type of class gets constructed ever time you receive a packet. I find in this case performance a valid argument. The best use would be to just call the constructor directly. Second best option would be to call the parameterless constructor via Activator. The worst case scenario would be to call Type.GetConstructor. At compile time the generic new constraint translates to the Activator. Another unexplored solution might be to use expressions, but not sure if that would be simpler/understandable.

Implementation is for @Damien to decide. The Invoke() method is not slow. GetConstructor() is (relatively!) slow though you could cache the result of that at startup. Then once you've got Invoke, its the same speed as a regular constructor call. If you're already worrying about performance at this time, and if the decision now is going to permanently affect your whole code base, then you're doing it wrong as it would indicate that it is not easy to refactor or replace / substiture parts of your code. Also I think he is already using the Activator anyways (or he has to re-use the packet instances which I certainly hope he isn't doing...)

Other benefits of using the such construction would be the (pre-)compile time checks the IDE and/or compiler gives you. You're also avoiding exceptions at runtime, because you're guaranteed to have a parameterless constructor.

If you're really worried about compile time, use code generators to generate the code for you.

Your solution wouldn't work because you still need to know the specific type on which you want to call the static method. Defeating the purpose of a generic implementation. Another drawback is testability. Static methods are a pain in the ass to test and should be avoid. Static methods are hardly ever the solutions.

Shouldn't really matter. You could just call the method with a bunch of data wrapped in an IClientMessage and observe the result. You just don't have a specific object and its own logic behavior, which you just said it doesn't need, to possibly affect the result. 1 + 1 is pretty static to me and could be tested too.

Solution to avoid this type of initialing is to use a dedicated parser class that specifically constructs that data structure. Does the extra class, and thus complexity really make it better tho?
I agree such method containing side effects should be avoid, but in this case it's short of justified.

Could do something along the lines of:

/// Probably has some lookup table of header / packet type (IEventArgs) somewhere I assume.
var packetDefinitions = new Dictionary<ushort, Type>();
var factories = packetDefinitions.ToDictionary(kvp => kvp.Key,  kvp => kvp.Value.GetConstructor(new[] { typeof(IClientMessage) }));

var packetInstance = factories[4000].Invoke(message);

 

[Joopie]

Great, then make it a struct.

No, because it would violate a few characteristics of the

You must be registered for see links

when to use a struct or not.

Actually, no. The data has already been received (At least I really hope). As the message has a prefixed length + header thus I assume that IClientMessage contains all the bytestream bytes already.

In theory yes, but malformed packets can still cause unexpected behavior.

Implementation is for @Damien to decide. The Invoke() method is not slow. GetConstructor() is (relatively!) slow though you could cache the result of that at startup. Then once you've got Invoke, its the same speed as a regular constructor call.

The Invoke() method is

You must be registered for see links

. It does not gain the same speed as by directly calling the constructor. It never will. The closest thing to get to that same speed is using compiled expressions. The invoke method still performs validation to check given parameters.

If you're already worrying about performance at this time, and if the decision now is going to permanently affect your whole code base, then you're doing it wrong as it would indicate that it is not easy to refactor or replace / substiture parts of your code.

I wouldn't say worried. Just something to think about when using reflection. I also wouldn't say reflection is the right answer here. It would just have as much of an impact on refactoring/replacing code.
Refactoring the code to avoid having the hard required method is just as simple as extracting the method into it's own class, name it something something deserialiser and we basically solved the argument.

Speaking about code should be done more anyway. Hiding it away doesn't make it better either. When arguing about code, better solutions follow and knowledge is shared.

Also I think he is already using the Activator anyways (or he has to re-use the packet instances which I certainly hope he isn't doing...)

Yes, I think he's using the Activator. Speedblood is given credit for helping with the "core". My 3+ years old code base might be of influence. The latter would have been fun to see tho.

Shouldn't really matter. You could just call the method with a bunch of data wrapped in an IClientMessage and observe the result.

I think I don't have a clue on what you actually mean here.

 

[TheGeneral]

I had a look at his repo. Looks promising. Better than some other stuff I recently saw. He'll be fine.

 

[Oni]

I like that! There are some things that could definitely be improved, but as you said you are still at the beginning of the project and I would love to help you with it.

I forked your repository to rewrite the base, unfortunately, I couldn't manage to take over any/all of your classes in time, because it's very late and I'm tired. But the database as well as the configuration part is already finished!
Tomorrow I will be happy to go into every single point, why I use which package, etc.
I will also continue and make sure to comment the code as much as possible:

You must be registered for see links

 

[Damien]

I like that! There are some things that could definitely be improved, but as you said you are still at the beginning of the project and I would love to help you with it.

I forked your repository to rewrite the base, unfortunately, I couldn't manage to take over any/all of your classes in time, because it's very late and I'm tired. But the database as well as the configuration part is already finished!
Tomorrow I will be happy to go into every single point, why I use which package, etc.
I will also continue and make sure to comment the code as much as possible:

You must be registered for see links

Looks good. When I'm awake I'll take the time to have a proper look. Thank you