Sphereserver / Source-X

Ultima Online server emulator
Apache License 2.0
57 stars 45 forks source link

Couple of Fixes #1222

Closed xwerswoodx closed 5 months ago

xwerswoodx commented 6 months ago
GladieUO commented 6 months ago
  1. Fixed: Missing fCheckOnly checks block the taming skill. (Issue: https://github.com/Sphereserver/Source-X/issues/1219) --> Taming works, followerslots are always 1 (combination of both sphere.ini flags activated), not sure if its intended or not. I assume that If I activate both flags, it should read the followerslots from chars.

  2. Fixed: The wrong input of Kill trigger, to make it compatible with older sphere versions. (Issue: https://github.com/Sphereserver/Source-X/issues/1210) --> WORKS

  3. Fixed: ARGN1 (reap amount) doesn't updated depends on the amount left on bit. (Issue: https://github.com/Sphereserver/Source-X/issues/1224) --> WORKS

xwerswoodx commented 6 months ago
  1. Fixed: Missing fCheckOnly checks block the taming skill. (Issue: Taming and OF_PetSlots bug? #1219) --> Taming works, followerslots are always 1 (combination of both sphere.ini flags activated), not sure if its intended or not. I assume that If I activate both flags, it should read the followerslots from chars.

It because FOLLOWERSLOTS is an old system, to get the total follower amount in new system, you need to use CURFOLLOWER command directly, .xshow CURFOLLOWER gives the follower count in the list, FOLLOWERSLOTS is only works if new system is disabled. You can use CURFOLLOWER command instead of FOLLOWERSLOTS as CURFOLLOWER returns the FOLLOWERSLOTS if new system is disabled.

GladieUO commented 5 months ago

Damn this PR will be fat AF 🤣 👌

Jhobean commented 5 months ago

Damn this PR will be fat AF 🤣 👌

Look like a branch and not a PR

xwerswoodx commented 5 months ago

Honestly important thing is making sphere ready for X1 as soon as possible, I tried my best to fix issues I could manage to fix, but there are still couple of issues need to be fixed before release

cbnolok commented 5 months ago

That's excellent work! Could you please wait a little before adding more code to this PR? It will be a mammoth to review (and debug, if ever needed) :P

xwerswoodx commented 5 months ago

That's excellent work! Could you please wait a little before adding more code to this PR? It will be a mammoth to review (and debug, if ever needed) :P

Sure, just updating the things for review.

GladieUO commented 5 months ago

Just tried the latest version of PR and most of my system are not working. There is something different with tags returning 00 instead of 0 and same with local.x returning 01 if it should be 1. 😲 Is this intended?

xwerswoodx commented 5 months ago

Just tried the latest version of PR and most of my system are not working. There is something different with tags returning 00 instead of 0 and same with local.x returning 01 if it should be 1. 😲 Is this intended?

Yes, they should return in hex like older versions, it was a mistake that we changed it to decimal, it wasn't intentionally, so you can use dTAG to get them decimal like older versions. We discussed it on here;

https://github.com/Sphereserver/Source-X/pull/1222#discussion_r1576632688

Jhobean commented 5 months ago

Yes, they should return in hex like older versions, it was a mistake that we changed it to decimal, it wasn't intentionally, so you can use dTAG to get them decimal like older versions. We discussed it on here;

I liked the new X behavior... No really agree to revert back...

xwerswoodx commented 5 months ago

I liked the new X behavior... No really agree to revert back...

It's understandable, we can discuss about it, honestly using different system between versions, causes many script-side issues, so I agree it cause many issues when we change it in middle of development, but honestly I can't decide this by myself.

Jhobean commented 5 months ago

Since day one i'm on X I tought it was a new feature and changed all my script. Impact is big on dialog and when you see visual value or when you compare two data. I talked about thia 4 years ago and it was say it was like this. I appreciate all work you do when bug is fixed but this change make frustrating huge difference when coding shard.

GladieUO commented 5 months ago

Im curious how it was on 56b and 56c since I dont remember we ever needed to use anything else than just simple check for tag.xxx returning decimals. Tbh taking some functionalities from the old spheres is always iffy, since we all know they were all over the place.

If in the X its working for years some way, mistake or not I think everyone using X is now used to it and cant imagine anyone appreciating this change. For servers in process, not launched, OK extra annoying work to change everything and check that it works again. But for someone already running live shard and wanting to keep with the updates, this can break a lot of stuff.

Whatever the decision will be, i guess we adapt to it and still appreciate rest of your work. 👍

Additionally I guess we can always revert this change for ourselves and recompile everytime there is new update.

cbnolok commented 5 months ago

Also i'd like to discuss issue #1171 since it's addressed in this pr

cbnolok commented 5 months ago

Im curious how it was on 56b and 56c since I dont remember we ever needed to use anything else than just simple check for tag.xxx returning decimals. Tbh taking some functionalities from the old spheres is always iffy, since we all know they were all over the place.

If in the X its working for years some way, mistake or not I think everyone using X is now used to it and cant imagine anyone appreciating this change. For servers in process, not launched, OK extra annoying work to change everything and check that it works again. But for someone already running live shard and wanting to keep with the updates, this can break a lot of stuff.

Whatever the decision will be, i guess we adapt to it and still appreciate rest of your work. 👍

Additionally I guess we can always revert this change for ourselves and recompile everytime there is new update.

Really does that change break so much stuff? I'm quite sure that on 56d and before everything returned hex values. That was the reason for implementing in the past dtag, dlocal etc. I don't remember when i changed that part of code, but i can say that change wasn't from the start of the fork. I see the benefits of both returning hex and decimal, i'd say to keep the historical behavior but i'd like to understand how this revert would actually impact shards. Obviously some changes are needed to be done on scripts, but really that much?

cbnolok commented 5 months ago

Once we sort out this and the new targ spellflag, i think this pr is ready for being merged

xwerswoodx commented 5 months ago

Once we sort out this and the new targ spellflag, i think this pr is ready for being merged

I revert back the spellflag update as SPELLFLAG_FX_TARG flag enough for event, I tested it and seems like effects works with spellflag_fx flags instead of spellflag_targ flags.

For decimal/hexadecimal return, we were discussing it in discord channel.

GladieUO commented 5 months ago

Really does that change break so much stuff? I'm quite sure that on 56d and before everything returned hex values. That was the reason for implementing in the past dtag, dlocal etc. I don't remember when i changed that part of code, but i can say that change wasn't from the start of the fork. I see the benefits of both returning hex and decimal, i'd say to keep the historical behavior but i'd like to understand how this revert would actually impact shards. Obviously some changes are needed to be done on scripts, but really that much?

From the first quick look, it broken all the dialogs using any kind of tags or locals. Which in general is telling me, that I will need to go through every single tag and local condition, test ingame if its work, and if not add one letter to make it work. For me personaly that means go through everything ive been working on for year and half and marked as done and tested, test and check again. I personaly dont see any benefits from using 16 as default, but im just lil pup to understand the purpose.

As I said, thats just me, if the change is needed or should have been done long time ago, we shall adapt.

xwerswoodx commented 5 months ago

Really does that change break so much stuff? I'm quite sure that on 56d and before everything returned hex values. That was the reason for implementing in the past dtag, dlocal etc. I don't remember when i changed that part of code, but i can say that change wasn't from the start of the fork. I see the benefits of both returning hex and decimal, i'd say to keep the historical behavior but i'd like to understand how this revert would actually impact shards. Obviously some changes are needed to be done on scripts, but really that much?

From the first quick look, it broken all the dialogs using any kind of tags or locals. Which in general is telling me, that I will need to go through every single tag and local condition, test ingame if its work, and if not add one letter to make it work. For me personaly that means go through everything ive been working on for year and half and marked as done and tested, test and check again. I personaly dont see any benefits from using 16 as default, but im just lil pup to understand the purpose.

As I said, thats just me, if the change is needed or should have been done long time ago, we shall adapt.

We decided to add h keyword (hTAG, hLOCAL, etc.) and an ini switch for default return, you can add DecimalVariables=1 in sphere.ini to make sure TAGs and LOCALs return as decimal instead of hexadecimal.