OnlineCop / kq-fork

Fork of KQ r910. Just for fun.
GNU General Public License v2.0
15 stars 9 forks source link

Preparatory work for eResistance fix #141

Closed OnlineCop closed 2 years ago

OnlineCop commented 2 years ago

I Tried Really, Really Hard...

Yay. One more massive update that changes all the things, all over the place.

Organization

I tried my best to break the commits down into individual "change only related things" chunks, like renaming a variable, or adding comments for clarification.

Common Names

Spells (s_spell), items (s_item), and fighters (KFighter) (usually used in the context of party[...]) each have items that can cast (or cause) elemental effects on the target. I renamed these to spell_elemental_effect, item_elemental_effect, and weapon_elemental_effect, respectively.

Now For the Oh-No!

Something to note with this commit: the arrays defined in res.cpp have not been changed in this commit. That's an upcoming PR.

This means that the data values may be all wonky!

However, the next PR, which actually changes the values in res.cpp is following closely on the heels of this one.

Since the player (and therefore, the save games) are probably using values that were originally also a blend of 0-based and 1-based for a lot of these stats, you may need to start a fresh game in order to do thorough testing, before reporting bugs.

pedro-w commented 2 years ago

I may have got a bit mixed up but R_TOTAL_RES means "no elemental effect" doesn't it? In which case should we rename it to something more descriptive? I noticed that most enums have got a NUM_XXX // always last line, apart from eResistance. How about

    R_NONE = 16,

    NUM_RESISTANCES = R_NONE // always last

for consistency

OnlineCop commented 2 years ago

I may have got a bit mixed up but R_TOTAL_RES means "no elemental effect" doesn't it? In which case should we rename it to something more descriptive? I noticed that most enums have got a NUM_XXX // always last line, apart from eResistance. How about

    R_NONE = 16,

    NUM_RESISTANCES = R_NONE // always last

for consistency

I think this approach would be fine. We still need NUM_RESISTANCES since it's used as array size initialization, but would you (or anyone else) be averse to renaming all of the last elements in enums as <PREFIX>_TOTAL?

Basically, all the other enum values share a common prefix (with only 2-3 notable exceptions), so it seems like it would make more sense to have the same prefix for the "last"/"count" element in the enums.

pedro-w commented 2 years ago

would you (or anyone else) be averse to renaming all of the last elements in enums as <PREFIX>_TOTAL?

I'm not really bothered but I think if it's NUM_XXX then it's clearly not one of the enum members. Say if I saw B_TOTAL I might think it's one of the bubble styles

OnlineCop commented 2 years ago

To keep the number of commits/changes to this PR as minimal as possible, I'm fine adding the R_NONE to the enum and some of the equality checks, but I'm going to avoid all the other changes till after this is merged.

I hate submitting PRs with dozens of commits like this, since it's a nightmare for the 2-3 of us to wade through.