Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for skills and attributes #1526

Merged
merged 8 commits into from Dec 11, 2018

Conversation

Projects
None yet
5 participants
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Dec 5, 2018

Contains a commit from #1394. Also Sormat battle commit 3.16, which somehow got lost in the rebasing.

Sormat's observation that items that trigger skills always set the source battler as the highest level member of the party. If there are multiple members of the party with the highest level, the first is selected. From testing I found his observations to be correct, with the additional caveat that the source also has to be able to act (not dead or has a nomove state).

In RPG_RT, this logic is applied a bit too heavy handedly and has a bug. Skills with usage of "Self" also get their source set to the highest level member of the party. Since the target is self, that means the target also gets forced to the highest level member! When you use such an item the first actor is highlighted, you can't select another actor, and the effect is always on the highest leveled party member regardless.

I opted to break compatibility for self skills here. Instead I let you use them on anyone, with the source being that same actor instead of the highest level member.

I tested all of this by changing the levels of party members and hacking their SPI stats really high or low to determine which character was being used as the source.

The idea behind the highest level actor logic makes sense but its not very good. For example, if your highest level actor is a fighter type with low SPI and then you use an item which cases a healing spell that has SPI effect, it'll be wasted as your high level fighter's SPI gets used.


if (effect < 0)
effect = 0;

This comment has been minimized.

@fmatthew5876

fmatthew5876 Dec 5, 2018

Author Contributor

I really don't like to see this logic duplicated here and in the battle code.

This comment has been minimized.

@fmatthew5876

fmatthew5876 Dec 5, 2018

Author Contributor

We should refactor this skill and item effect logic in some common place. I will do this only after battle PRs are merged.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:skill_menu2 branch 2 times, most recently from 7f1b93c to 3afc69c Dec 5, 2018

@fdelapena fdelapena added the Battle label Dec 5, 2018

@fdelapena fdelapena added this to the 0.6.0 (likely) milestone Dec 5, 2018

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:skill_menu2 branch from 3afc69c to bc2c44f Dec 6, 2018

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:skill_menu2 branch from 64329da to 6bc3f4a Dec 6, 2018

Show resolved Hide resolved src/game_battler.cpp Outdated
@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 6, 2018

Leaving a note that I need to test what happens when highest level actors cannot use the item. What if Target cannot use?

Albeleon and others added some commits Jul 11, 2018

Operations to UseSkill with source; GetHighestLeveledActor for menu s…
…pecial items.

Problem 1: Skills used in the menu didn't take in factor attributes and the source's ATK and INT depending of the skill. Solution: Add a Source to the function and when a skill is used, pass the source actor. We need to add GetAttributeMultiplier to the Actor from Game_BattleAlgorithm to make this operation too. We also take in account whether a character is resurrected with skill->affect_hp being false (the base value will cure HP percentage in that case), using "cure_hp_percentage".

Problem 2: When using a special item, the parameters it takes are from the highest leveled character in the group. In case there are many, it takes the first one. We pass Game_Party::GetHighestLeveledActor as parameter.
Correct attribute multiplier - Albeleon 3.16
* Handles physical and magical as RPG_RT
* Previously Albeleon 3.16, which seemed to be lost in
  rebasing.
Fix targeting in menu for items with skills
* Fixes items with skill target party
If skill has Scope_Self, source should be self
* RPG_RT forces you to always use items with self skills on the highest
  level actor. We break compatibility and allow you use the item on
  anyone, but since its a self only skill they will be the source and
  target.
Fix item usablility in menu
* Usable is based on the "user" of the item.
* For non-skill items, target == source
* For skill items, highest level actor must be able
  to use the item.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:skill_menu2 branch from 6bc3f4a to cb5760f Dec 7, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 7, 2018

Turns out item usability plays into item skills.

If the highest level actor can't use the item, then the next highest level actor is chosen as the "source".

I've moved the clamping damage values commit to #1452

This is ready now. I don't have anything more to add.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 11, 2018

At least for "One Ally" and "Whole Party" it doesn't seem to be "Highest Leveled Actor".

When the Skill is of Magical Element and you set it to E (0%) for 3 of the 4 party members then only the one member who doesn't have E will be healt by the item.

Because that one member isn't the highest leveled actor the caster must be the actor himself.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 11, 2018

When the Skill is of Magical Element and you set it to E (0%) for 3 of the 4 party members then only the one member who doesn't have E will be healt by the item.

But that's because elements are based on the target, not the caster. Try it with magical influence and super high/low SPI stats. That's how I tested it and it really is highest level actor.

Also we have this 50% thing in our healing skill effect logic. Maybe its wrong?

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 11, 2018

ah that makes sense, but something must be wrong in our healing calculation because the healing should be 0. I will try with that 50% patched out.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 11, 2018

I determined via testing that this 50% is nonsense in both map and battle...

@Ghabry
Copy link
Member

Ghabry left a comment

Will accept this when the obvious wrong 50% conditions are removed.
The calculation still looks wrong, I get different amounts compared to RPG_RT in some cases but this could be part of another, smaller PR (and need to prepare a test case).

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 11, 2018

Removed the the 50% thing.

I wouldn't worry too much about perfect accuracy at this stage. We have still more battle PRs which adjust the skill effect logic. After all are in I'd like to test for accuracy and merge the skill effect logic from battle and menu to one place.

@Ghabry

Ghabry approved these changes Dec 11, 2018

@Ghabry Ghabry merged commit 175809c into EasyRPG:master Dec 11, 2018

7 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Wii (SDL1) Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details

@fmatthew5876 fmatthew5876 deleted the fmatthew5876:skill_menu2 branch Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.