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.

Sign up for GitHub

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

Jump to bottom

Refactor Skill and Item usability and fix some incompatibilities #2380

Merged
merged 5 commits into from
Oct 11, 2020
Merged

Refactor Skill and Item usability and fix some incompatibilities #2380

merged 5 commits into from
Oct 11, 2020

Conversation

mateofio
Copy link
Contributor

@mateofio mateofio commented Oct 5, 2020 •
edited
Loading

Taken from RE disassembly, which is a bit messy and hard to read here. Needs to be tested to verify.

Tests

Skill Usability Tests

  • Test all 3 Escape conditions
  • Test all 3 teleport conditions
  • Test switch skill + field and battle flags
  • Test states which limit physical and magical rate
  • Test all 5 scopes
  • Test affect none, hp only, mp only, both hp and mp
  • Test heals state which persists after battle
  • Test heals state which ends after battle
  • Test skill which has 1 physical attribute with and w/o equipment
  • Test skill which has 2 physical attribute with and w/o equipment
  • Test skill which shifts physical attribute with and w/o equipment
  • Test all above as skill in map / menu
  • Test all above as skill in battle
  • Test all above as skill item in map / menu
  • Test all above as skill item in battle
  • Test all above as equipment invoke skill item in map / menu
  • Test all above as equipment invoke skill item in battle

Item Usability Tests:

  • Medicine usability
  • Common and Equipment usability
  • Skill book
  • Seed
  • Switch item - with field and battle activiation flags
  • Live party member which can use item required for normal and subskill item skills but not escape, teleport, or switch

RPG_RT bugs ❗

See here: #1486 (comment)

@mateofio mateofio changed the title Refactor IsSkillUsable Refactor IsSkillUsable and fix some incompatibilities Oct 5, 2020
@mateofio mateofio requested a review from a user October 5, 2020 01:16
@mateofio mateofio changed the title Refactor IsSkillUsable and fix some incompatibilities Refactor Skill and Item usability and fix some incompatibilities Oct 5, 2020
@mateofio mateofio marked this pull request as ready for review October 5, 2020 02:20
@mateofio
Copy link
Contributor Author

mateofio commented Oct 5, 2020

I've tested all the edge cases listed in the original post with a test game. This is ready.

@mateofio mateofio mentioned this pull request Oct 5, 2020
Closed
@fdelapena fdelapena added this to the 0.6.3 milestone Oct 5, 2020
@mateofio mateofio mentioned this pull request Oct 5, 2020
Merged
5 tasks
@mateofio mateofio force-pushed the skilluse branch 2 times, most recently from 5c26db0 to 7f06d23 Compare October 9, 2020 12:37
@fdelapena fdelapena requested a review from Ghabry October 9, 2020 18:23
@mateofio
Refactor IsSkillUsable
cbd169b 
Fixes the following bugs:
* Physical attribute weapon checks skipped if skill changes attribute
* Physical attribute weapon checks do not require 2 weapons flag
* Physical attribute weapon checks require item type is weapon
@mateofio
IsItemUsable() - use Algo::IsSkillUsable
a663436
@mateofio
Remove assert on escape/teleport skills in battle
163d2ba 
These can be triggered by equipment which invokes skills,
and they do nothing
@mateofio
Add additional party usability checks required for skill items
6f7f52f
@mateofio
Use Algo::IsNormalOrSubskill everywhere
3cf4bd6
@Ghabry
Copy link
Member

Ghabry commented Oct 11, 2020

@fmatthew5876
To sum this up: What were the biggest bug fixes here? The checkbox-list is not very helpful for a blog post later because it doesn't differentiate between "worked before" and "works now" :)

Ghabry
Ghabry reviewed Oct 11, 2020
View reviewed changes
src/game_party.cpp
@@ -255,10 +255,6 @@ bool Game_Party::IsItemUsable(int item_id, const Game_Actor* target) const {
return false;
}

if (data.party.size() == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does removing of this check have any practical use? You can't open the item scene in the menu when the party is empty and there is no event command to use items.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not, but having it has no practical use either. It wasn't in RPG_RT so I took it out.

@mateofio
Copy link
Contributor Author

TBH I don't have a full list. I just decided to clean this up and make it match RPG_RT.

A few that come to mind:

Probably more things got fixed, but I didn't test master thoroughly to compare

@Ghabry
Copy link
Member

Ghabry commented Oct 11, 2020

hm okay. The next blog will be full of "we changed something but nobody really knows what it fixed" ^^' (looking at you, Interpreter 80 commits refactor)

@Ghabry Ghabry merged commit b0eba25 into EasyRPG:master Oct 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ghabry Ghabry Ghabry left review comments

@fdelapena fdelapena fdelapena approved these changes

Assignees
No one assigned
Labels
Battle Refactor
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mateofio @Ghabry @fdelapena