Sphereserver / Source-X

Ultima Online server emulator
Apache License 2.0
58 stars 47 forks source link

Couple of Fixes and Feature Updates #1120

Closed xwerswoodx closed 1 year ago

xwerswoodx commented 1 year ago
cbnolok commented 1 year ago

Aren't karma/famechanged redundant triggers? In karmachange i could get the old value with and the new with karma + argn1

xwerswoodx commented 1 year ago

Aren't karma/famechanged redundant triggers? In karmachange i could get the old value with and the new with karma + argn1

The problem is argn1 is writeable, and it can be changed after on_trigger. And setting local after karmachange triggered not affect local.new value. Because we have to send arguments before argn1 changed so after argn1 changed it's impossible to update Local.new for trigger.

For example, if you .set karma 100 and your karma is 50, you get old=50 new=100 and argn1=50 its fine but when you use argn1=200 under trigger, it returns old=50 new=100 and argn1=200 because argn1 updated after on_trigger but we have to send args inside on_trigger function so to update args we have to send again but this can cause endless loop as player have argn1=200 inside trigger so it keep try to trigger endlessly to update everytime. Or I don't know if there is a way to update local variables after trigret returned.

I meant we sent the arguments for trigger inside OnTrigger void at line 632 but if argn1 changed the local.new argument should changed at line 635

cbnolok commented 1 year ago

I mean, do we really need a double trigger and a local.old/new when we could just do karma + argn1? Or, we could set argn1 = current karma (read-only), argn2 = karma change.

xwerswoodx commented 1 year ago

I mean, do we really need a double trigger and a local.old/new when we could just do karma + argn1? Or, we could set argn1 = current karma (read-only), argn2 = karma change.

Honestly for me it's totally useless, even we didn't have local.old and new before and I have never look for it, because as you said, we can get these values by functions easily. But they added local.old and new, and as they accepted, I didn't want to remove it directly because may someone use it like Soulless using them.

For me, while src.karma already giving old karma and argn1 should give karma change (which is broken in current build) and karma+argn1 should be enough to track all values, but I don't really know why local.new and old added at all. I read the change log and I made a change to make it how they should be, but removing them is not a big deal for me, as I said I just keep them because they already added.

But if you think it's useless, and shouldn't need to have I can remove them in a minute, or I can send only old value and value to change instead of new karma, and people can still use local.old + argn1 to get new karma, but local.old still useless because src.karma updated after trigger finished so src.karma should already give local.old without any issue. so src.karma + argn1 should be enough.

cbnolok commented 1 year ago

I get your point, but i'd rather remove them than adding a new trigger :D

xwerswoodx commented 1 year ago

I get your point, but i'd rather remove them than adding a new trigger :D

I removed new trigger and send local.old in argn2 directly if anyone wish to use :))

Tolokio commented 1 year ago

Ill try to find time this weekend to test everything. nice work.