Sadhu skills #302

felipecastagnarodecarvalho · 2024-08-16T17:34:58Z

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.

…sadhu_skills

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.

src/ZoneServer/Buffs/Handlers/Clerics/Sadhu/OOBE_Anila_Buff.cs

src/ZoneServer/World/Maps/Map.cs

src/ZoneServer/World/Actors/CombatEntities/Components/MovementComponent.cs

src/ZoneServer/Network/Send.cs

exectails · 2024-09-16T11:00:55Z

src/ZoneServer/World/Actors/Characters/Character.cs

 			if (!ZoneServer.Instance.Data.MapDb.TryFind(mapId, out var map))
 				throw new ArgumentException("Map '" + mapId + "' not found in data.");

+			if (this.IsOutOfBody())			


I feel like this is a weird place to check for out of body to prevent warps, because what if we do want to warp? This special behavior also isn't documented anywhere, so anyone who might try to warp someone will have to dig deep into the warp code to figure out why it's not working.

Additionally, creating explicit checks for very specific conditions within general purpose functions is not ideal, because it's not extensible. An alternative idea that I'm just going to throw out there might be to add a warp lock, which you can set for these buffs you're checking in IsOutOfBody, which will generalize the whole approach, make it somewhat self-documenting, save you from having to do that manual buff check, and have it be reusable.

You marked this as resolved, but the issues are still there 🤔

warping prevention removed!

src/ZoneServer/World/Actors/CombatEntities/Components/MovementComponent.cs

system/scripts/zone/ais/sadhu_dummy.cs

system/scripts/zone/core/calc_combat.cs

exectails · 2024-09-16T11:22:42Z

system/scripts/zone/other/character_adv.cs

 			{
 				LearnSkill(character, SkillId.Hammer_Attack);
 				LearnSkill(character, SkillId.Hammer_Attack_TH);
-				LearnSkill(character, SkillId.Sadhu_OutofBodyCancel);


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

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

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.

This WAS from the official data, but it got outdated, since the last changes on sadhu, the skill is not being used anymore.

That may be true, but it is in fact still part of the job data as of the last time I checked, which this script is based on.

Removing this makes updating the data more challenging, because you have to remember not to add this skill. Potential automated tools for this purpose would also have to be changed not to re-add it. And if it is actually still used for something without our knowledge, we would rather not remove it. Not to mention that any kind of backwards compatibility we might want to introduce would potentially require this line to be re-added.

For these reasons I asked whether there are any issues with leaving it as is, because that is the simplest solution for my concerns, unless we have to conditionally remove it for some reason.

This is skill is not being used anywhere.
The class got reworked at some point and the skill got removed.

"Sadhu_OutofBodyCancel" => To cancel from being out of Body you now need to press the use the same skill you used first time.

This image shows the actual behavior.

src/ZoneServer/Buffs/Handlers/Clerics/Sadhu/OOBE_Anila_Buff.cs

src/ZoneServer/Skills/Handlers/Clerics/Sadhu/Sadhu_Skill_Base.cs

src/ZoneServer/Network/Send.Normal.cs

src/ZoneServer/Skills/Handlers/Clerics/Sadhu/Sadhu_Anila.cs

src/ZoneServer/Skills/Handlers/Clerics/Sadhu/Sadhu_Skill_Base.cs

exectails · 2024-10-03T22:29:41Z

src/ZoneServer/World/Actors/Characters/Character.cs

 			if (!ZoneServer.Instance.Data.MapDb.TryFind(mapId, out var map))
 				throw new ArgumentException("Map '" + mapId + "' not found in data.");

+			if (this.IsOutOfBody())			


You marked this as resolved, but the issues are still there 🤔

system/scripts/zone/ais/sadhu_dummy.cs

felipecastagnarodecarvalho · 2025-03-08T06:30:13Z

@exectails this PR is ready to be reviewed again.
I have put some effort into improving the code.

exectails

I appreciate your efforts for improving the code, though some concerns have not been cleared yet. Still, I might consider merging the PR soon, assuming you're still around and able to maintain the changes, fixing issues in a timely manner should they arise.

Given the nature of this class, the code is unfortunately not very straightforward to follow in its current form, and I suspect potential changes or improvements to it would be simpler to accomplish if one were to rewrite the skills. As their developer, I assume you would have a better overview of them and their behavior.

exectails · 2025-05-20T19:24:48Z

src/ZoneServer/Network/Send.cs

+		/// </summary>
+		/// <param name="character"></param>
+		/// <param name="targetCharacter"></param>
+		public static void ZC_UPDATED_PCAPPEARANCE(Character character, Character targetCharacter)


You've added a few overloads like this, where you differentiate between a character and target, but several of them don't seem to serve a real purpose. Like this one, where all the information is taken from the target, but the source of the broadcast is a different character. This could lead to confusion over what information the caller is expected to give. It could also lead to visual bugs, because if someone else is in range of the target character, but not the character, they might not receive the packets you're broadcasting. These senders should get reevaluated and cleaned up to only make this distinction if that is actually necessary and desired.

exectails · 2025-05-20T19:41:40Z

src/ZoneServer/Network/Send.cs

 		/// <param name="propertyList"></param>
 		public static void ZC_OBJECT_PROPERTY(IZoneConnection conn, long objectId, PropertyList propertyList)
 		{
+			// TODO: Find a better way to check this - This was done to avoid sending packets for dummies (while receiving buffs)


I don't recall if I mentioned this before, but the idea behind a dummy anything is usually that you can use them like the real thing without thinking twice about it. As such, if you have a dummy character, would it not be easier to also have a dummy connection, to be able to "send" them packets without actually doing so? Or is there a reason I'm missing for why a null check might be preferable?

I don't think we would need a fake connection for dummies.
A connection means that a client exists behind it, right?
For the other side, we can send packets to the dummy map:

dummy.Map.Broadcast(packet);

If in the future the dummy plays an animation (when the dummy is selling equipament improvements and it needs to play the refinement animation, for example) we can use this without problem.

I have move this check to InitEvents() method on CharacterProperties.cs:

/// <summary> /// Sets up event subscriptions, to react to actions of the /// character with property updates. /// </summary> private void InitEvents() { // This was done to avoid sending packets for dummies (while receiving buffs) if (this.Character.Connection == null) return; [...] }

exectails · 2025-05-20T19:43:56Z

src/ZoneServer/Network/Send.cs

-			// [i373230] Maybe added earlier
+			// [i373230] Maybe added earlier [Updated at 28/07/24]
 			{
-				packet.PutByte(0);


Were you able to confirm that this is actually a short and not two bytes? Changing data like this could easily lead to confusion at a later point when we try to figure out what these bytes do.

Changes reverted.

exectails · 2025-05-20T19:56:33Z

src/ZoneServer/Network/Send.cs

+		/// </summary>
+		/// <param name="entity"></param>
+		/// <param name="target"></param>
+		public static void ZC_MSPD(ICombatEntity entity, ICombatEntity target)


Is it possible you're misunderstanding what this sender does? While this overload allows you to treat the two values as distinct entities, the only difference is who gets notified about the change. Generally speaking, you'd want the source entity to also be the target entity, so the other clients in range of the target get informed about the target's new speed--not the clients who are close to the source, who might not even be in visible range of the target.

exectails · 2025-05-20T20:01:02Z

src/ZoneServer/Pads/Handlers/Cleric/Sadhu/Sadhu_Anila_Effect_Pad.cs

+		/// <param name="creator"></param>
+		private async Task MovePad(Pad pad, ICombatEntity creator)
+		{
+			// Forward and back, hovering a moment in between.


The comment says forward and back but that's not what's happening here. Could you clarify whether the error is in the code or the comment? If it's the comment that's incorrect, and the pad only moves forward, this comment could safely be removed, as the method appears clear-cut and is commented appropriately.

wrong commentary. Fix applied.

exectails · 2025-05-20T20:10:07Z

src/ZoneServer/World/Actors/Characters/DummyCharacter.cs

+		/// <summary>
+		/// Returns true if the DummyCharacter has Owner
+		/// </summary>
+		public bool hasOwner => Owner != null;


There are still some convention violations, such as curly braces on the same line as a foreach or this camelCase property. If you're using Visual Studio, auto-formatting should solve some of these issues and the style checker should inform you about the rest.

exectails · 2025-05-20T20:11:58Z

src/ZoneServer/World/Actors/CombatEntities/Components/CooldownComponent.cs

 			}

-			if (this.Entity is Character character)
+			if (this.Entity is Character character && character is not DummyCharacter)


You're mixing conventions of checking for dummy characters before calling senders and inside them, as well as mixing type and null checks. I do understand what led to this, but it would be better to stay consistent. Better yet would be to not need this distinction.

I have changed to:

if (this.Entity is Character character && this.Entity is not DummyCharacter) Send.ZC_COOLDOWN_CHANGED(character, cooldown);

But it's essentially the same. It's needed since the dummy casts a skill.

exectails · 2025-05-20T20:15:36Z

src/ZoneServer/World/Actors/CombatEntities/Components/MovementComponent.cs


 				Send.ZC_MSPD(this.Entity);
 			}
+			else if (this.Entity is Character character && character is DummyCharacter dummyCharacter)


I would presume this check to be either incorrect or redundant. Am I right in that assessment? You're checking whether the entity is a character and a dummy character, which would mean the code applies only to dummy characters, as only they are both. Though in that case it would be enough to check for the dummy character type. And if you wanted it to apply to both, a check for character would be enough, as the dummy inherits character iirc.

exectails · 2025-05-20T20:27:11Z

src/ZoneServer/World/Maps/Map.cs

 			{
 				foreach (var character in _characters.Values)
-					character.Connection.Send(packet);
+					character.Connection?.Send(packet);


I'm not a huge fan of this change, as it introduces a new type of character into the system that did not exist prior and might not be handled correctly in all cases: a character without connection. Before this, all characters could be presumed to have a connection during their entire session, from when they connect to when they disconnect. If that's no longer the case, we'd have to make sure that any access to the connection property, as well as information behind that, such as accounts, are properly checked before they're accessed.

While I recognize that this only applies to the Sadhu dummy characters right now, and that everything may very well work as intended for the moment, we will inevitably use the cloning methods you introduced for other purposes and other systems are going to have to work around these dummy characters, regardless of where they came from.

With that in mind, have you made sure that this radical change won't cause any issues in the existing code?

This is a bit confused for me.
Last time we talked you said that it would be better to keep using the on going Characters instead of creating a new dictionary:

private readonly Dictionary<int, Character> _dummiesCharacter = new();

That what I did last time and I have refactored to keep using the current dictionary.
I am not sure if this approach will work still with vending dummies character but it should!
It would require a few changes but mainly it's the same.

exectails · 2025-05-20T20:39:22Z

system/scripts/zone/other/character_adv.cs

 			{
 				LearnSkill(character, SkillId.Hammer_Attack);
 				LearnSkill(character, SkillId.Hammer_Attack_TH);
-				LearnSkill(character, SkillId.Sadhu_OutofBodyCancel);


That may be true, but it is in fact still part of the job data as of the last time I checked, which this script is based on.

Removing this makes updating the data more challenging, because you have to remember not to add this skill. Potential automated tools for this purpose would also have to be changed not to re-add it. And if it is actually still used for something without our knowledge, we would rather not remove it. Not to mention that any kind of backwards compatibility we might want to introduce would potentially require this line to be re-added.

For these reasons I asked whether there are any issues with leaving it as is, because that is the simplest solution for my concerns, unless we have to conditionally remove it for some reason.

…sadhu_skills

felipecastagnarodecarvalho · 2025-07-14T22:25:01Z

@exectails I have update this PR with:

Correct OOBE animation for the dummy.
Improvements on the AI for the cloned character (can be used if you have [Arts] Spirit Expert: Wandering Soul).
Fixed all mentioned commentaries.
Fixed a bug that the monster wouldn't be alerted by the cloned character when taking damage.

felipecastagnarodecarvalho added 22 commits July 27, 2024 02:32

Initial Sadhu skill's implementations

9d14ab5

[WIP] Sadhu skills

0f1cfaa

Merge branch 'master' of https://github.com/NoCode-NoLife/melia into …

8a0803a

…sadhu_skills

Adding the first sadhu Skill - Prakriti

cdf1740

Polishing Prakriti

01a2f3a

Adding Anila skill

bd6a41f

Adding Possession skill (sadhu) + skill base class inheritance

3c5e170

Adding Enira sadhu skill

80ff162

Working on the sadhu arts that creates clones

5aa0295

Small Update (Sadhu)

020d514

Merge branch 'master' into sadhu_skills

cef4651

[WIP] Sadhu skills tweek

70535a7

Messing up with the IA (for dummies)

e2203f0

Minor changes Sadhu

d3fba4d

Minor update sadhu

60bc776

Merging master

507317e

Updating sadhu dummy AI

10cda3d

Merging master

f2b09d5

Sadhu skills - Working on the hability skills

2c6492c

Adding last two Sadhu skills

9ede990

Updating skill file names

e6f87ac

Merging master

888b8d5

exectails requested changes Sep 16, 2024

View reviewed changes

felipecastagnarodecarvalho added 3 commits September 17, 2024 18:33

Merge branch 'master' into sadhu_skills

d55e4af

[WIP

73e8c55

Resolving review conversations

1a49524

exectails requested changes Oct 3, 2024

View reviewed changes

felipecastagnarodecarvalho added 3 commits March 6, 2025 17:35

Merging Upstream Master

08bde3b

[WIP] Fixing review issues

a0075b3

Fixing remaining review issues

86338ef

exectails requested changes May 20, 2025

View reviewed changes

felipecastagnarodecarvalho added 5 commits June 21, 2025 11:55

Initial fixes for review

3c9ed92

Merge branch 'master' of https://github.com/NoCode-NoLife/Melia into …

579e41d

…sadhu_skills

Updating sadhu skills (2) - commiting before pulling from master

9c4d791

Merge branch 'master' of https://github.com/NoCode-NoLife/Melia into …

adadead

…sadhu_skills

Ready for a new review - sadhu updates

845a5f0

felipecastagnarodecarvalho requested a review from exectails July 17, 2025 13:50

Sadhu skills #302

Are you sure you want to change the base?

Sadhu skills #302

Uh oh!

Conversation

felipecastagnarodecarvalho commented Aug 16, 2024

Uh oh!

exectails left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felipecastagnarodecarvalho commented Mar 8, 2025

Uh oh!

exectails left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!