Status Effect refactoring for cleaner code about hercules HOT 12 OPEN

I'm actually strongly against this change. The proposed solution has too many implications. only to avoid setting one value in the SC configuration.

The current implementation offers a lot of flexibility (like the already mentioned SC_AUTOTRADE) that we would need to reimplement, with the added burden for server admins to manually update their database to not screw things up.

A lot of things can go wrong for little to no benefit.

I fully agree with with what @csnv stated. I'm againt this change. The flexibility of choosing what "status icon information" to send to the client is an advantage that one shouldn't remove.

I think it's fine to refactor and split the HandicapStates and BodyStates from the SC_ logic though, although this is likely to cause duplicated code due to the timer stuff and such as mentioned by @guilherme-gm ( timer creation/processing/deletion ), if not done in an elegant way.

from hercules.

I'm actually strongly against this change. The proposed solution has too many implications. only to avoid setting one value in the SC configuration.

The current implementation offers a lot of flexibility (like the already mentioned SC_AUTOTRADE) that we would need to reimplement, with the added burden for server admins to manually update their database to not screw things up.

A lot of things can go wrong for little to no benefit.

@guilherme-gm I'm not sure but I think SC values are required to be defined in constants_db.conf so conf files can use those constants instead of plain numbers. It's the same with skill enums.

from hercules.

@csnv the thing is, the source can insert constants directly, without the need of constants_db.conf, many constants (e.g. script commands constants, even item_db ones) are defined directly from source, so it uses the value from source: https://github.com/HerculesWS/Hercules/blob/stable/src/map/script.c#L29227

This function runs right after constants db is read. Loc values from item_db, for example, are defined in there.

-- Edit:

I would say doing that is safer than the current approach, because right now you can mess up the source id vs script/db id, automatically defining them would ensure they are always in sync.

-- End of edit

We could even use XMacro to automatically keep the list filled... maybe I will open a PR later with that

from hercules.

This change isn't trivial. Some SC (like SC_STONE) don't match any SI icon, what ID do we assign those? If we are not careful, Gravity can add new SI ids client side that we thought were free and used for SC without icons, introducing new bugs that wouldn't be there with the current method.

from hercules.

I also have some other concerns about this:

1. As hemagx pointed in herc post, the current method is there to save memory. EFST goes up to 1100, while we have ~800 SCs, I am not sure how much of this gap is missing content vs gaps created by official servers. If there are real gaps, we will be allocating memory for nothing
2. Creating custom sc will be messier. The id stored in db comes from SC value, if we follow SI, new official SI will conflict with custom and you will need to do batch updates to your db (while today you could simply add the official one after your custom and just change the SI sent to client

from hercules.

@csnv As I said, effects that don't have an icon use another enumeration

SC_STONE = BODY_STONECURSE

Remembering that the status effect is different from the effect like SC_STONE, so it would be easily possible to use an icon if you want to show the SC_STONE effect, just call the stone effect and a status stone effect, even some effects do this in the aegis

@guilherme-gm

1 . I disagree, since we have almost 1600 entries, remembering that we have to count the SC and the SI
2 . I disagree that it will be more confusing, since you only need to create an SC and place it in the client's lua and in the server's enumeration, there will be no need to create the SI_ anymore, today's model, this is confusing, because you have to create an SC, then an SI and then in the client lua and as for updating the custom effects, this is very simple, nothing complicated since you will only need to change the id in the emulator and in lua, we have already had changes that required much more to implement custom things, and I highly doubt that any server has more 50 custom effects

from hercules.

1. What I mean is sc->data[].

enums doesn't really use memory, and constants should use a small amount and it is a 1-time allocation.

But sc->data is allocated per player. (here: https://github.com/HerculesWS/Hercules/blob/stable/src/map/status.h#L1219 )

Today we have 719 SCs, which makes each player allocate an array of 719 pointers. As far as I am aware, there are no arrays allocated for SIs. ( I remember messing this up when I released RoDEX -- #1817 )

If we switched to SI right now, this number would grow to 1149 positions per player (if we follow herc constants) or 1457 based on the linked lua. Are those 430 (or 738) slots just missing content? or will we be allocating much more than we need?

From a quick scan on the linked lua, I can see several ID gaps (for example, we have only 1258 lines, while the higher id being 1457, so these ~200 slots may be unused)

--

2. For this part, I meant SQL db. the SC id is stored in the database, sc_data table. Let's say I have my herc copy with the following:

	SC_SOULDIVISION,

	SC_ACTIVE_MONSTER_TRANSFORM,

	SC_FIRE_EXPANSION_TEAR_GAS_SOB, // 718

        SC_MY_CUSTOM_SC1, // 719
        SC_MY_CUSTOM_SC2, // 720

#ifndef SC_MAX
	SC_MAX, //Automatically updated max, used in for's to check we are within bounds.
#endif
} sc_type;

this means that my sc_data table will have entries for ids 719 and 720 for those custom SCs whenever a player has those SCs active.

If herc gets a new official SC today, I can merge the update like that:

	SC_SOULDIVISION,

	SC_ACTIVE_MONSTER_TRANSFORM,

	SC_FIRE_EXPANSION_TEAR_GAS_SOB, // 718

        SC_MY_CUSTOM_SC1, // 719
        SC_MY_CUSTOM_SC2, // 720

        SC_NEW_HERC_SC, // 721
#ifndef SC_MAX
	SC_MAX, //Automatically updated max, used in for's to check we are within bounds.
#endif
} sc_type;

and for my server, this new official SC will have id 721 internally/in db, while exposing the right SI to the client. (if there was a SI conflict, I would need to adjust it too, but this is still at client)

On the other hand, if I only have SIs, I would need to:

1. update the IDs of my custom effects in server + client lua
2. update my database accordingly (this may be a +N in the IDs, if everything goes well) -- but this can't be forgotten

if I forgot step 2, my players who had my custom effects would now be using the other effects instead. Note that I am considering here a live server with real data. a test server where I can simply wipe everything won't be affected by this case.

if we are willing to leave people forget to update their databases and deal with the results of that, I think this point can indeed be ignored.

from hercules.

@guilherme-gm Yes, now I understand what you mean, in relation to sc->data[], we just don't have the same amount as lua because the development of the emulator is slower, but one day we will reach that same value.

If we implement this now, the growth should be according to Hercules and add new ones as the mechanics are implemented

As for the sc_data table, a sql script changing the current ids to the official ids can be added, as for custom we can also offer a script that changes the old ID to the new ID, remembering that the custom one must be executed first before the official one.

We can also write code in the emulator to detect if the server has any custom effects, we can prevent the emulator from running until it updates the effect

There are other ways too, I think if implemented correctly it won't cause problems

from hercules.

Ok, if they are all real SIs and not gaps that will never be filled, then I think it is ok.

I like the idea of having some sort of mechanism to detect inconsistencies to prevent the server from starting in an inconsistent state.

One last question:

If I understood the enum of body state/handicap state etc, this enum is meant for those states that are not real buffs, but that are related to the character options. And I think those are what are called "common ailments" today, which goes at the start of the enum.

There are some SCs that are Hercules specific, like SC_AUTOTRADE and SC_KSPROTECTED, those are not in the "common ailments" category nor have an official SI for them. Following your proposal, where those would go to?

from hercules.

@guilherme-gm exactly these are body effects, some of which are at the beginning of the enumeration, others such as EFFECT_HANBOK, EFFECT_SUMMER among others are not at the beginning

Regarding the current effects of SC_AUTOTRADE and SC_KSPROTECTED, I think they can be converted to the map_session_data variable, any other ideas are welcome

from hercules.

I see. I don't have other ideas... using SC for things that lasted for some time like autotrade, jail and ks protection has been a pattern, as far as I know, since the SC "engine" would automatically handle timers, data allocation/deallocation and persistence between sessions.

From the top of my head I don't have any other suggestion.

It will probably require some new code to handle those things (timer creation/processing/deletion) and new db columns too, since some of those are persisted between sessions (e.g. jail time, autotrade timeout), but may work. I don't know all SCs that are hercules specific

from hercules.

I forgot to add on my previous posts, so (adding as a new comment since it has been a few hours and may be go unnoticed if I just edit)

I still think this change doesn't bring benefits, because:

Our gains are: we stop having 2 IDs (SC vs SI) -- saves the need to keep a list of constants and copy/pasting them when needed

At the cost of:

1. we are no longer able to "abuse" the SC mechanism to make timed effects on units (like autotrade timeout, ks protection, jail time) -- Any custom inclusion in herc that needs a timer will now require their own code since they can't be made with custom SCs
2. things that used to be in SC are now split into several places (SI + wherever body state / health state / etc goes + other settings that don't belong to either)

I won't hard oppose it, though.

To me, SI being "I want my SC to be sent to client with this ID" and simply be the client interface works well, and I still get SC as the internal mechanism to do all sorts of stuff (like the examples above for jail/autotrade/ks protection).

One thing that I do think would improve SCs is moving their constants out of constants_db.conf and back to source, like everything else source-defined is (on script.c, script_hardcoded_constants).

Today we have to set the enum + constants_db conf, and this can easily get out of sync, SCs are the only one that goes that way and I am not sure if there is a reason for it.

from hercules.