NoCode-NoLife / melia

Open-Source MMORPG Server Emulator
GNU General Public License v3.0
265 stars 94 forks source link

Refactor calculated properties #218

Open exectails opened 1 year ago

exectails commented 1 year ago

There are properties that are calculated based on other properties, and to make this more efficient, we had decided to only recalculate them when necessary, but I think we need to reevaluate this approach, because it can be difficult to wrap your head around and it's very error prone. It's easy to forget to set up auto-updates, and when you get the value of a property, you kind of expect it to be the correct one, but that just isn't guaranteed currently.

I think we should switch to calculating everything on the fly for now and see where that gets us. If we operate under the assumption that we always get the correct value when requesting it, this won't cause any additional work down the line, even if we decide to cache values once more.

Additionally, there should be a simple way to replace these calculation functions from scripts, since users might want to adjust some of them.

exectails commented 1 year ago

After adding skill properties, including calculated ones, I converted them to referenced ones today, because there was a situation where I needed to invalidate a skill property when a buff's overbuff counter changed, which I found a little convoluted. Only calculating properties when necessary is neat in theory, but when you never know what might affect them, because users can customize the formulas, it becomes a matter of invaliding everything whenever anything changes, in which case everything is constantly recalculated anyway. Whether we'll make the switch for characters as well is yet TBD though.

exectails commented 7 months ago

For mine and others' future reference.

There is another alternative to how we could more or less easily manage dependencies, by using attributes. For example, the following scriptable function uses several character properties, and it needs to be recalculated whenever one of these dependencies changes.

[ScriptableFunction]
public float SCR_Get_Character_STR(Character character)
{
    var properties = character.Properties;

    var byJob = properties.GetFloat(PropertyName.STR_JOB);
    var byStat = properties.GetFloat(PropertyName.STR_STAT);
    var byAdd = properties.GetFloat(PropertyName.STR_ADD);

    var result = byJob + byStat + byAdd;
    return (float)Math.Floor(Math.Max(1, result));
}

Our current approach for accomplishing this is to set up automatic updates like this.

this.AutoUpdate(PropertyName.STR, new[] { PropertyName.STR_JOB, PropertyName.STR_STAT, PropertyName.STR_ADD });

This works well enough, as long as nobody tries to make STR dependent on DEX or some other factor we didn't set up, because that would require them to go deep into the character properties setup and add this dependency to the list. A way we could technically make this more flexible would be to move the dependencies to the actual scriptable function.

[ScriptableFunction]
[Dependencies(PropertyName.STR_JOB, PropertyName.STR_STAT, PropertyName.STR_ADD)]
public float SCR_Get_Character_STR(Character character)
{
    var properties = character.Properties;

    var byJob = properties.GetFloat(PropertyName.STR_JOB);
    var byStat = properties.GetFloat(PropertyName.STR_STAT);
    var byAdd = properties.GetFloat(PropertyName.STR_ADD);

    var result = byJob + byStat + byAdd;
    return (float)Math.Floor(Math.Max(1, result));
}

During setup we would simply read the dependencies attribute and set up the necessary auto-updates, giving users and scripters control over this aspect. And since the information is right there, the chance for someone to forget about it should be relatively low.

But I'm still not thrilled about needing an additional step that needs to be made, documented, and understood. Additionally, we would have to set up all kinds of dependency attributes, because you need not only properties, but also information about buffs, equipped items, enabled features, abilities, and so on. Not to mention everything a user might come up with, like doubling STR on a monday during a solar eclipse.

As a result, I'm still thinking auto-updates aren't really viable for us right now.