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

Albeleons's Battle fixes 1 of N #1447

Merged
merged 22 commits into from Nov 4, 2018

Conversation

Projects
None yet
5 participants
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Oct 14, 2018

Here is the first batch of rebased fixes from #1373

All of these are pretty simple changes so I'm making a first PR with these. They are not all in the same order as originally presented. I'm trying to pull out the ones that are simple and don't require a lot of merging so they can be quickly reviewed and merged into master.

Changes made:

  • 2.7 - Using Player::escape_char instead of "\\"
  • 4.1 - Battle_BGM was moved earlier so don't play it twice.
  • 2.15 - Added back const
  • 2.4 - Enhanced Game_Actor::GetSkillName() with skill selection logic.

These alone fix some issues from #1418 and #985

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_1 branch 3 times, most recently from 9ef56de to 057a464 Oct 15, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Oct 16, 2018

I've reviewed these commits and done some basic testing. They all look good to me 👍

@Albeleon

This comment has been minimized.

Copy link
Member

Albeleon commented Oct 16, 2018

2.7: I remember there were some complications with the "\" and "Player::escapeChar" after trying to change the codification from Occidental to Japanese that made me go back to "\" again. I'll put a pin on this and recheck comparing both again.

4,1: The reason why PlayBGM was kept twice is because the battle music should sound also when you only do a battle test (without opening the map), and it doesn't reach Scene_Map::CallBattle in that case. I considered there was no problem with a double call because the function PlayBGM doesn't take much workaround if the song is the same with the same conditions. But this is up to discussion.

2.15: I don't remember why I took out "const", maybe it was during tests trying things to make it work and I just kept it, or there was a definition problem? I don't remember ^^u, I hope it was just a mistake for my part, but just in case later I'll recheck that commit to see whether there is a difference or not.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Oct 16, 2018

@Albeleon why don't you take these over again? You have a much better understanding of your changes than me.

I can close out my PRs

EDIT: I've left 1, and 2 of my set open. Up to you how you'd like to proceed but if you need help merging this with your other big PR's I can at least continue with these.

Reverted 2.7 and 4.1

@fmatthew5876 fmatthew5876 reopened this Oct 17, 2018

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_1 branch 4 times, most recently from 32cec2c to 9770419 Oct 17, 2018

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Oct 27, 2018

needs a rebase

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_1 branch from 9770419 to 43c232b Oct 27, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Oct 27, 2018

Rebased

@@ -980,7 +980,7 @@ bool Scene_Battle_Rpg2k3::CheckWin() {
std::vector<int> drops;
Main_Data::game_enemyparty->GenerateDrops(drops);

Game_Message::texts.push_back(Data::terms.victory);
Game_Message::texts.push_back(Data::terms.victory + "\\|");

This comment has been minimized.

@Ghabry

Ghabry Oct 27, 2018

Member

2.7: RM2000 + RM2003: The Victory message should have pauses after...

I'm almost sure that these "\"-codes must be replaced with Player::escape_symbol, otherwise they won't be recognized in japanese or korean games.

@@ -102,9 +102,7 @@ void Scene_Battle_Rpg2k::CreateBattleCommandWindow() {
}

void Scene_Battle_Rpg2k::RefreshCommandWindow() {
std::string skill_name = active_actor->GetSkillName();
command_window->SetItemText(1,

This comment has been minimized.

@Ghabry

Ghabry Oct 27, 2018

Member

2.4: RM2000 (not tested in RM2003): If you have the "special skill name...

Style question: As this is only used in combination with GetSkill what do you think about moving this "IsSkillRenamed" logic directly in "GetSkillName"?

This comment has been minimized.

@fmatthew5876

fmatthew5876 Oct 27, 2018

Author Contributor

I think that makes sense. From Player perspective there is only 1 skill name. The renaming stuff is a liblcf detail that should be abstracted away.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_1 branch 2 times, most recently from 96821c1 to fa974cf Oct 27, 2018

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Oct 27, 2018

Cool had some problems with it. Also we talked about it in irc a while ago. I haven't tested it.

I have no idea what the original problem was but replacing "\\" with Player::escape_symbol works for me.

You can use encoding "ibm-943_P130-1999" (Japanese with Yen as backslash) for testing.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_1 branch from fa974cf to f4df746 Oct 27, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Oct 27, 2018

I've addressed the issues and updated the list of changes in the top comment of this PR.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_1 branch from f4df746 to 493e52d Oct 27, 2018

@Ghabry

Ghabry approved these changes Oct 27, 2018

@fdelapena fdelapena added the Battle label Oct 28, 2018

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_1 branch from 493e52d to e583369 Nov 1, 2018

Albeleon added some commits Jun 14, 2018

"1.2: RM2000: Animations executed in a battle event should not wait u…
…ntil the animation is over when the option "wait to end" is selected."

Solution: Eliminate in Game_Interpreter_Battle::ExecuteCommand and "if" that checks whether the animation is finished no matter if the option "wait to finish" is clicked or not. This difference is already treated and differentiated by itself, and this condition is just a left-over code from previous function structure that isn't needed anymore.
"2.7: RM2000 + RM2003: The Victory message should have pauses after e…
…ach line. The first line with the victory message has a longer pause than the others (in RM2003 only the first line should have a pause)."

Solution: Add after each "string" to be printed in the victory message:
- "\|" to the first message in RM2000 and RM2003.
- "\." to the following messages in RM2000.
"4.1: RM2000 + RM2003: Battle music should start when the TransitionI…
…n from map to battle starts instead of when it ends."

Solution: Add, when a battle is called before the TransitionIn, a BgmStop and BgmPlay with the battle music.
"4.3: RM2000 (not tested in RM2003): If you press "Cancel" when an at…
…tack is being made, the "Cancel" sound shouldn't play."

Solution: Scene_Battle_Rpg2K::ProcessInput made the "CANCEL" sound play everytime you press the button no matter in which state the Battle is in. It's been fixed to only sound in the places where it has an operation.
"4.5: RM2000 + RM2003: The "DECISION" sound when pressing the Attack …
…command should only sound once, not twice."

Solution: Scene_Battle::AttackSelected already has the "DECISION" sound. Therefore, it's not needed for it to sound when the command is selected. We eliminate it then for the "Attack" command in the Scene_Battle_Rpg2k::CommandSelected and Scene_Battle_Rpg2k3::CommandSelected (for the other commands (skill, items, defend, etc.) it's still kept).
"4.6: RM2000 + RM2003: There should be a "DECISION" sound when you pr…
…ess a target (enemy or ally)."

Solution: We add a "DECISION" sound when an ally or enemy is selected in Scene_Battle.
"4.7: RM2000: The Escape sound should sound after the message and whe…
…n the TransitionOut starts, instead of before the message."

Solution: Move the "ESCAPE" sound from the beginning to after the message is finished (successfully) and before the Scene is popped.
"4.8: RM2000 + RM2003: When exiting via F12 (Screen Title) in a battl…
…e, the TransitionOut should be a quick fadeout, not the same slow defined BattleErased one."

Solution: Add in Scene_Battle::TransitionOut an "if" that checks if the type of the current instance (after the scenes have been popped) is Scene::Title.
"2.4: RM2000 (not tested in RM2003): If you have the "special skill n…
…ame" (to give your character a different name to the skill command) checkbox unmarked, it should show the default skill name instead of what is inside."

Solution: Instead of checking whether the skill rename is empty, it checks whether the rename_skill checkbox was clicked or not (after all, if it's clicked, you can put any name, even an empty one).
For this, we modify Game_Actor::GetSkillName() to always return the
correct skill.
"4.9: RM2000 + RM2003: The "DECISION" sound when pressing the Defend …
…command should only sound once, not twice."

Solution: Taking away the sound when selecting "Defend" in Scene_Battle_Rpg2k and Scene_Battle_Rpg2k3 (it's already played in Scene_Battle).
"2.13: RM2000: When pressing CANCEL when selecting an enemy target, i…
…t should return to the previous window / state, not to SelectCommand. Just like AllyTarget."

Solution: in Scene_Battle_Rpg2k::ProcessInput (and 2k3), change the state of EnemyTarget with AllyTarget, where it returns to the previous state.
"2.12: RM2000: Using an object that restores SP displays the message …
…with the amount -1 (it recovers the normal SP though)."

Solution: Fix an error where in the GetHpSpRecoveredMessage it took GetAffectedHp instead of value (which changes for HP or SP). It returned -1 because Ethers only cure SP.

By checking this, it's also been fixed "GetHpSpAbsorbedMessage" where it printed Data::terms.health_points instead of the input parameter "points" (which means, if you absorb SP, it would still say it was HP in the message).
"4.4: RM2000 (not tested in RM2003): When you use an Item, the "Item"…
… sound should play."

Solution: Fix an error where in Game_BattleAlgorithm::Item::GetStartSe it only played if the object was of type switch (which actually doesn't generate sound) instead of medicine.

Also, fix another error in the same function: the SFX_EnemyAttacks would never be executed since an enemy can't use items, so it's always NULL.
3.3 + 3.5 (together because they have some common code):
"3.3; RM2000: An absorb attack shouldn't make the enemy flash and generate "Damage" sound."
"3.5: RM2000: If a skill decreases stats that are not HP, the enemy shouldn't flash and the "Damage" sound shouldn't play."

Solution for 3.3: Put a condition so it doesn't flash or generate sound. For this:
1. We create a public bool function IsAbsorb() in AlgorithmBase which returns the bool "absorb", so it can be used outside.
2. Change every instance where absorb is used to put IsAbsorb (this is done for consistency since other parameters use the same).
3. To not generate the sound: in AlgorithmBase::GetResultSe, if the attack is absorb, it returns NULL.
4. To not generate the flash: in Scene_Battle_Rpg2k::ProcessBattleAction, in the BattleActionState_Result, when the enemy flashes, it should be put as a condition that the attack shouldn't be absorb.

Solution for 3.5: Put a condition so it doesn't flash or generate sound. We know if GetAffectedHp is -1, HP is not a parameter, so we'll use this check:
1. To not generate the sound: in AlgorithmBase::GetResultSe, if the successful attack doesn't affect HP, it returns NULL.
2. To not generate the sound: in Scene_Battle_Rpg2k::ProcessBattleAction, in the BattleActionState_Result, when the enemy flashes, it should be put as a condition.

Also added that the attack shouldn't be IsPositive(): it shouldn't flash if the enemy is healed.
"2.9: RM2000: When an ally receives damage result that is more than 0…
…, the screen should shake a little bit."

Solution: Add a screen shake when a sucessful damage is caused to an ally target, with damage that is more than 0, and is not positive or absorb.
"1.3: RM2000 + RM2003: When an enemy dies in a event, it has differen…
…t effects depending of whether it dies by HP damage or status condition = Death. In the first one, it gradually disappears with monster dying sound. In the second one, it just disappears inmediately."

Solution:
 - When in Game_Interpreter_Battle::CommandChangeMonsterHP the enemy dies, generate the dying monster sound.
 - When in Game_Interpreter_Battle::CommandChangeMonsterCondition the enemies suffers Death, its graphic disappears inmediately, so we don't see the progressive disappearance.
 - When an enemy is dead, ChangeHP shouldn't work.
 - To resurrect an enemy, you use RemoveCondition Death. In that case the enemy appears inmediately and with 1 HP. This has been implemented too.
"2.15: RM2000: The conditions caused by a weapon attack are different…
… from the ones selected in RPG_RT (i.e. a weapon with Poison and Blindness, causes Death)."

Solution: This happened because when calculating which state it was, they put state_set[i], which is not the answer because state_set is a set of booleans that mark if the state i is checked or not. Therefore, because the conditional before said it's checked, state_set[i] => i + 1 (because the ID starts with 1 instead of 0).

Also, since an enemy isn't inmediately in Death condition when taking the Death Message, and it's the only message that matters, we change "GetTarget()->GetSignificantState()" for "ReaderUtil::GetElement(Data::states, 1)" (always Death).
"2.16: RM2000: If you have one instance of an object and it's the las…
…t one in the list, in the battle, when you use it and return to the item window, for a frame you can see the cursor in that position (which now it's empty and shouldn't be accesible) before going back to a normal position. Logically it's fixed, but graphically it's an awkward frame change."

Solution: Put SetIndex in Windows_Item instead of changing the index directly. This will update the cursors and it will automatically clamp it.
"2.19: RM2000: When going to the skill battle menu, only when you go …
…from SelectCommand the index should go to the first one (0). If you press CANCEL after selecting an enemy, it should keep in the same position."

Solution: in SetState, only SetIndex(0) if the previous state was SelectCommand.
"2.20: RM2000: Only active enemies should have their actions defined …
…for a turn: hidden or dead ones shouldn't."

Solution: In "CreateEnemyActions": Change "GetBattlers" for "GetActiveBattlers."
"3.6: RM2000: If an enemy doesn't have any more stats to decrease by …
…a skill (and HP is not there), the enemy should "dodge" that attack. If the enemy has at least one stat to decrease, but other who doesn't (and is not HP), that message shouldn't show up at all, instead of appearing as 0."

Solution:
1. In "Game_BattleAlgorithm::Skill::Execute", a skill fails if no buff/debuff stat is bigger than 0. If HP is selected, it always succeeds unless it's a negative absorb damage and HP is 0. If SP is selected, it only fails if SP is 0 and the attack is not positive.
2. In GetResultMessages, not print the buff/debuff stats that aren't bigger than 0. When SP is 0, only print it when the action is positive.
"3.8: RM2000: Buffs and bebuffs should be able to stack the more you …
…use them. Also, there is a limit: the modified stats (decrease or increase) only go in the range [-ParameterBase / 2, ParameterBase * 2]. In case the buff/debuff overcomes the limit, it should be clamped and same with the message result. Heal skills (HP and SP) also clamp with the difference healed (so, if Ally 22/30 HP, and uses a potion or heal, it will only show 8)."

Solution:
1. The reason why the buffs/debuffs don't stack is because they use "SetXXXModifier" instead of "ChangeXXXModifier", it sets the value instead of add it or decrease it. So we create those methods in Game_Battler. We change them in "Apply".
2. We clamp the buffs and debuffs within that range in Skill::Execute. If the skill is positive, HP and SP are also clamped to not heal more than the HP or SP left.
3. If HP is an absorb negative, it is clamped to not take away more than the enemy's current HP. This also means moving the absorb set earlier.
4. We put the same HP and SP clamp for healing medicines.
5. When a buff / debuff stat is absorbed from the target, these should also be clamped in the same way they were in point 2.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_1 branch from e583369 to 3f881c6 Nov 3, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 3, 2018

Possible this and #1394 can merged this weekend? Of all the PRs out there Sormats are the most important in my opinion.

They are large and likely will introduce regressions. We will need time after merging everything to test the new master before the next release.

@carstene1ns carstene1ns merged commit b7475ac into EasyRPG:master Nov 4, 2018

6 checks passed

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

@fmatthew5876 fmatthew5876 deleted the fmatthew5876:albeleon_battle_1 branch Nov 5, 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.