miwarnec / uMMORPG_CE

4 stars 0 forks source link

Item cooldown doesn't persist logout #18

Closed iamallama closed 3 years ago

iamallama commented 4 years ago

The item cooldown that is part of the UsableItem class isn't saved to the database and so when you logout and back in, the cooldown is reset. This becomes a problem when you have an item that has a cooldown longer than a few seconds.

burnydev commented 4 years ago

Specially long cooldown that need relogin protection should be handle by buff. As not every item would benefit of that new database field and need many condition check.

here example: https://youtu.be/1oQqRq4VmwM

iamallama commented 4 years ago

The existing system doesn't work as intended, unless that intent was to only allow item cooldowns of 10 seconds or less as they could be bypassed by relogging. So by definition, it's a bug as it works in unexpected or unintended ways.

Using the buff system would work, but that is not it's intended purpose to be used as cooldown timers for things unrelated to player status effects. Every buff gets checked when stuff like damage, defense, etc occur and it would add more overhead when you have several long lasting item cooldowns that need to be checked that don't apply to the player's status. Adding a status icon to the buff bar would also make it harder to understand what is going on with status effects to your character when you have several "cooldown" buffs cluttering the list.

burnydev commented 4 years ago

item cooldown should be only quick instance cooldown, something with very specific gameplay should not be hardcode for ALL item. like decay on all item ingame, is that so general that need a source change for all vis2k assets?

longer persistent cooldown should be on buff, as this class already have many methods to handle the long cooldown and many other aspect. if your potion have sickness debuf or overtime healing you cant have this on normal cooldown (without more modification to many system)

buff cooldown is by category and item is by item on each player

for all online player for each item at each save: if timedCooldownaActive, if timedCooldown > higherThanMyRelogin, if this cooldown must be protected... and maybe more compare to... maybe 10 - 20 new cooldown buff CATEGORY in a list?

and you could hide the bufficon with cooldown and only show cooldown on item to have the desired visual effect.

a game is like a movie, what happening in the background is often non realistic compare to what end user see, so the cooldown do not absolutely need to be on the actual item.

and for this particular case where its a potion .. the cooldown must be on the player if you want be able trade the potion without the cooldown, and a non tradable potion is very game specific! my game can trade long cooldown potion.

iamallama commented 4 years ago

I was actually wrong in discord about how the item cooldown works. It wouldn't need to be stored with each item, but should be it's own table with the category and time until the cooldown ends. Specifically it should be the data stored in Player.itemCooldowns.

Most of what you are describing are status effects, exactly what buffs are designed for. The item cooldown system that is built in and isn't persistent is more of a global cooldown to prevent reusing an item that falls within a specific category. That's it. No additional status effects, just a global timer that says you can't use an item again until after some cooldown ends.

A software bug is an error, flaw or fault in a computer program or system that causes it to produce an incorrect or unexpected result, or to behave in unintended ways.

https://en.wikipedia.org/wiki/Software_bug

The fact is, that given this widely accepted definition, this is something that behaves in a way that wasn't originally intended or is unexpected. If the intention is to only be a short lived, then it should be implemented like that. Get rid of the item cooldowns and use only buffs or use a Range attribute to limit the values that can be put on the cooldown setting and document it in the comments to say that this is only for short lived global cooldowns and doesn't persist to the database. Then the process of using a buff should be formalized and actually added to the UsableItem class. I'm personally fine with either option, I just don't see why there would need to be two systems to do the same thing.

Congrats that you found a workaround to what is a bug by definition. But when someone new to the asset sees something working in an unintended way, they are going to thinking it's broken, not that they should be using another system in a way it wasn't intended to be used.

burnydev commented 4 years ago

Again i do not see a bug here. If you want over use item cooldown to achieve other task .. up to you.

But you have other tool to achieve this.

So i have create an extension, not found a workaround. New user should be explained that cooldown are working like this, and IF they need long lasting persistent cooldown, they should use the tool made for this.

You paste wikipedia, you congratulate me, ... wow ... i think you are way out of subject.

If you bring real use case where you absolutely need the actual cooldown on the item, I would like to continue discussion.. Not base your tough on the fact that you think is broken. You cannot just say its broken because it do not do what you want, the way you want or you dont know how to use it.

Forgot to tell that I dont see the point of removing the non persistent quick cooldown to add way more overhead on buff!

Trugord commented 4 years ago

Good after having looked at several possible solution my idea would be to create a table "cooldown" which would manage the delays which must be it.

So create a new table "cooldown" with fields

with this method you can exchange, destroy, take back, or whatever you want the cooldown would not be destroyed and would be persistent as long as it is active.

Once the cooldown expired there and deleted nothing more, so you can manage the cooldown of all and anything object, item, quests or whatever

for not adding weight on cooldowns from 1 to 5 seconds for example just add a bool on the scriptable (stored (true / false) thus only important cooldowns such as buff, skill or long-lasting item would be recorded on the database

burnydev commented 4 years ago

I dont see the point to create a similar system when buff already do that.

The real improvement would be to be able use buff on item as cooldown to have long persistent cooldown on special item, just by dragging them on item and set time.

Dominus-Esports commented 4 years ago

Burny you are missing the point, buffs are used during combat calculations now imagen 1000ccu with standard buffs in place maybe lets say 8 and now you throw a couple of work around buffs for spells witch can be many I mean this is a MMORPG so 100's spells is a given but not all needs this persistent cd so lets say 20, that is a truckload of unnecesary calls and possible crashes unless buffs cean be exluded from calculations it is not a solution.

This is not a hard thing to fix just save and runs those already created float var's to the players ID and spell ID when the server loses connection to the player and by default the skills should mirror those var's when loging into the game and by Vis2k's standards showed so far keeping server integrety is a must to stop as many exploits as humaly posseble.

burnydev commented 4 years ago

Good point, all buffs are loop in many methods in the Skills.cs.

First, lets just check for damage. Not all the combat!

If we follow your point, just for the method GetDamageBonus(), all buff without damage should not be accepted.

So skill like Bessing, Therapy, Windwalk, Murderer, Offender, Health Aura... that have no damage or defense bonus is not so good. Because they are check in the buff loop in GetDamageBonus() and GetDefenceBonus(), on each combat hit, and they offer no bonuses for those. And its not the more frequently use methods, look at GetHealthBonus() and GatManaBonus().

So... why my solution for long persistent cooldown potion could bring HUGE inefficient calculation to THE MMO RPG Visk2k standards!

But you can think that I have miss the boat. (c8

miwarnec commented 3 years ago

hey guys, thanks for sharing the details. I agree it's a bug, didn't consider that possibility when adding Player.itemCooldowns.

we have two options: a) move back to cooldown as buffs (buff times are saved in DB). works and is the easiest solution, but it's a lot harder to use. creating a new buff for each item cooldown or category is very cumbersome. it's not fun at all b) simply save Player.itemCooldowns in database. this at least fixes the bug so that Player.itemCooldowns work properly. could still consider using buffs or not using buffs afterwards.

seems like b) is the most obvious move for now.

miwarnec commented 3 years ago

fix will be in V2.23. thanks for reporting!