Sadhu skills

[felipecastagnarodecarvalho]

All Sadhu Skills:

• Enira
• Moksha
• Patati
• Possession
• Prakriti
• SpiritExpert
• Tanoti

Also there was an implementation of the art Spirit Expert: Wandering Soul which creates a dummy that acts by himself.

[exectails]

Thanks for the PR. I took a quick glance and I noticed that some of it still seems a bit prototypical, and I also have some questions and concerns before we continue. Please refer to my comments.

[exectails]

Can you tell me more about your reasoning for creating a whole new category for dummy characters? Seeing how a dummy character is presumably the same as a normal character, with the only exception being that it's not controlled by a client, it seems like this might add a lot of unnecessary future maintenance work, because we will always have to remember to handle the dummies in some cases but not others.

To make matters worse, the game has a separate "Dummy PC" concept (ZC_ENTER_DUMMYPC). I don't know if that one is still in use, but if it is, and if they will require a category of their own, that will only complicate things even more.

[felipecastagnarodecarvalho]

Some of Melia methods takes in consideration that a character on the _characters dictionary have a connection and dummies don't have it. That should be the main difference. Well, I am out of ideas on how to improve it. I don't think it's should be overcomplicated: is it a dummy? Instead of adding to the _characters dictionary add it to the _dummies.

Broadcasts is an example that we should be threating characters differently from dummies:

		/// <summary>
		/// Broadcasts packet to all characters on map.
		/// </summary>
		/// <param name="packet"></param>
		public virtual void Broadcast(Packet packet)
		{
			lock (_characters)
			{
				foreach (var character in _characters.Values)
					character.Connection.Send(packet);
			}
		}

		/// <summary>
		/// Broadcasts packet to all characters on map, that are within
		/// visible range of source.
		/// </summary>
		/// <param name="packet">Packet to broadcast.</param>
		/// <param name="source">Reference actor for visible range to broadcast in.</param>
		/// <param name="includeSource">Send to source as well?</param>
		public virtual void Broadcast(Packet packet, IActor source, bool includeSource = true)
		{
			lock (_characters)
			{
				foreach (var character in _characters.Values.Where(a => (includeSource || a != source) && a.Position.InRange2D(source.Position, VisibleRange)))
					character.Connection.Send(packet);
			}
		}

[exectails]

Well, you're saying,

it's should be overcomplicated: is it a dummy? Instead of adding to the _characters dictionary add it to the _dummies.

but while duplicating dozens of lines of code is technically simple, it makes future maintenance and understanding the code much more complicated. And in a project that many more people will work on over many more years, that is far from ideal.

Instead, I would argue that an overall less complicated way is to view dummies as that which they are: characters without a connection. One way to handle that could be to check the character type before sending packets. But another, even more opaque way would be to give dummy characters a dummy connection. Simply a connection type that doesn't do anything when Send is called. That way it doesn't matter if you ever do send a packet for any reason, as it will effectively be ignored.

I would implement this, but looking at the current buff handlers, it's hard to tell how this would and should affect the existing code, so you would have to do it.

[exectails]

Can you tell me what the exact purpose of this else if is? It seems like you could just pass the character into ZC_MSPD, which would allow you to leave it as one if.

[felipecastagnarodecarvalho]

This is needed because DummyCharacters don't have connection so I need to pass the Owner to the other method.

[exectails]

The better place is the buff handlers, from where you can hook into all this without having to add the code here^^ Notice the SCR_Combat_AfterCalc call just above, which makes the server call the handlers of all active buffs to see if they'd like to change anything about this skill hit.

[exectails]

Can you tell me why you removed this? It's part of the official data, which there probably is a reason for.

[felipecastagnarodecarvalho]

This skill is not being used anymore and has no purposes. Sadhu has been remake I think.

[exectails]

This is based on official data though, and removing it creates a dissonance between server and client. Unless this causes issues, it should stay this way.

[exectails]

I gotta say, frankly, I'm a bit unhappy with these buff handlers. The code is pretty difficult to follow, especially with all the conditionals for dummy characters and lack of comments. Additionally, you're duplicating a lot of code even though you went to the trouble of adding a base handler class, which could handle much of it. Such as the entire OnActivate method, which is verbatim the same for all of them, the same one line to get the skill character, and more.

I would like to see these handlers receive a bit more structure and documentation, because as is, I can't follow any of this without reading, studying, and analyzing the entire code and the skill descriptions. And while this might be acceptable for the time being, at some point someone else will want to or have to change something about this job, and if they have to spend the same amount of time as you did initially writing this code, that's far from ideal.

[exectails]

Have you considered moving the creation code to the dummy character class, to decouple it from the skill handlers? While they aren't going to be used by anything else for now, I suspect we'll have different use cases for them, such as vendors or clones spawned for fun or events,

[exectails]

Tip:

var sadhuSkills = casterCharacter.Skills.GetList(a => a.Id != skill.Id && skillTreeData.Any(a => a.SkillId == skill.Id));
foreach(var availableSkill in sadhuSkills)
	Send.ZC_NORMAL.EnableUseSkillWhileOutOfBody(casterCharacter, buffId, (int)availableSkill.Id);

[exectails]

Unless there's a reason for making skillId an integer, you should rather use SkillId, for some type safety. Otherwise, any arbitrary integer, short or byte could easily be passed in.

[exectails]

While the buff ids might seem like they're just the buff class names, they aren't (necessarily). The names in the enum get modified slightly to prevent errors we would get if we used the class names as is in C#. For this sender that means it needs to get the actual class name from the buff data, because the buff id is not reliable. This might not actually be an issue with the Sadhu skills/buffs, but if their names should change, or other buffs use this packet as well, it would cause problems that would be difficult to diagnose.

[exectails]

Missing this., coding conventions, yada yada^^

[exectails]

It's unfortunately not very clear what's going on here if you're not deep into this job already, made worse by all the duplicated code between Handle and CreateClone. Like with the buff handlers, I'd like to see a bit more structure that is easier to follow here.

[exectails]

This code is redundant, as it's essentially what SetFixedMoveSpeed does.

[exectails]

You should not send raw packets from AI scripts unless there's absolutely no other way to accomplish what you need to do, because scripts, by their nature, should be simple to understand for users who are less experienced.

A cleaner solution would be to call some type of despawn method, which then internally figures out how to remove the dummy or whatever other actor the AI might be controlling.