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

Albeleon's Battle Fixes 3+4 of N #1449

Merged
merged 6 commits into from Nov 20, 2018

Conversation

Projects
None yet
4 participants
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Oct 15, 2018

A few larger commits

Depends on #1448

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_3 branch from 9b7da55 to c7ebc3f Oct 15, 2018

@fmatthew5876 fmatthew5876 reopened this Oct 27, 2018

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_3 branch 2 times, most recently from 6380045 to e584c37 Oct 27, 2018

@fdelapena fdelapena added the Battle label Oct 28, 2018

@Ghabry Ghabry added this to the 0.5.5 (or 0.6.0) -> we will see milestone Nov 4, 2018

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_3 branch 2 times, most recently from 31e799d to 80ab336 Nov 4, 2018

Show resolved Hide resolved src/scene_battle_rpg2k.cpp Outdated
Show resolved Hide resolved src/scene_battle.cpp Outdated
Show resolved Hide resolved src/window_base.cpp
Show resolved Hide resolved src/window_base.cpp Outdated
@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 5, 2018

(Also need to see the window animation in action to further comment on it)

Is it possible to detach these 4 commits conflict free from the others? (if not, keep it as is)

@Ghabry
Copy link
Member

Ghabry left a comment

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 5, 2018

(Also need to see the window animation in action to further comment on it)

Is it possible to detach these 4 commits conflict free from the others? (if not, keep it as is)

I tried and I start getting merge conflicts. I don't want to mess it up and get into rebase hell.

What we can do is move out anything questionable from #1448, merge those, and then rebase this onto master. It'll be much easier to work with then.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_3 branch 2 times, most recently from fd9e0cd to 5582564 Nov 8, 2018

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 8, 2018

The good news: 50% of the battle commits are in. The bad news: 50% of the battle commits are not in yet. 😅🙈

Thanks for your rebase work

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 11, 2018

I understand 2.1 now. Create a map event which puts sleep on the entire party and then start a battle. In master you'll see the battle menu and status window flicker for a frame, but in this branch you won't.

He must have tested the bug this way as it only fixes it in this case. There is still the problem of handling turn 0 troop events which either kill entire party to make the party unable to move. The core problem is there is never a frame during State_Start where battle events are allowed to process. This needs to happen after the battle is done displaying monster names.

Scene_Battle_Rpg2k::ProcessActions() changes the states immediately after its done displaying the "Monster Appeared" messages from Scene_Battle_Rpg2k::DisplayMonstersInMessageWindow(). There is a never a time when events / immobility check can process until after we have switched into State_SelectOption it's UI widgets have been drawn.

I need to study the battle code more to understand what is the best general solution for these cases:

  1. Party starts the battle unable to move (recoverable) -> skip menu and start battle sequence
  2. Party starts the battle unable to move (not recoverable) -> skip menu and start defeat sequence
  3. Turn 0 event kills the party or immobilizes (not recoverable) the party -> skip menu and start defeat sequence
  4. Turn 0 event immobilizes (recoverable) party -> skip menu and start battle sequence

Commit 2.1 in this PR only fixes (1) and (2).

I have not tested this in 2k3 mode, only 2k.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 14, 2018

I've dropped 2.1 and 2.10 in favor of #1497

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_3 branch from 6afb567 to 6751fa5 Nov 14, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 14, 2018

I've addressed Ghabry's review comments. Hopefully this is now ready to be merged.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 15, 2018

To reduce rebase mess I have merged battle 4 back into this PR.

I've also addressed Ghabry's comment about GetType() in 2.11 by adding a virtual. I don't like this solution either, but it's good enough to get through these PR's until we can do a proper refactor.

@fmatthew5876 fmatthew5876 changed the title Albeleon's Battle Fixes 3 of N Albeleon's Battle Fixes 3+4 of N Nov 15, 2018

@Ghabry
Copy link
Member

Ghabry left a comment

test

@Ghabry
Copy link
Member

Ghabry left a comment

Show resolved Hide resolved src/scene_battle.cpp
Show resolved Hide resolved src/game_battlealgorithm.cpp Outdated
}
}
else {
return skill.name;
}
}

bool Game_BattleAlgorithm::Skill::IsSecondStartMessage() const {
return Player::IsRPG2k() && (!item || item->using_message != 0) && !skill.using_message2.empty();

This comment has been minimized.

@Ghabry

Ghabry Nov 18, 2018

Member

2.5: RM2000: If the second line of a Skill is not empty

Grammar wise the name should be "HasSecondStartMessage".
This suggestion is maybe minimal more inefficient but the whole function could be removed and the code in battle_rpg2k.cpp replaced with an "empty()" check.

This comment has been minimized.

@fmatthew5876

fmatthew5876 Nov 18, 2018

Author Contributor

I changed it to HasSecondStartMessage(). It's called in 3 places in scene rpg2k. I think it's better to be behind a named function.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 18, 2018

the last 2 commits look okay.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_3 branch 3 times, most recently from f752606 to 7ced8d5 Nov 18, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 18, 2018

I've addressed all of the most recent set of review comments.

@Ghabry

Ghabry approved these changes Nov 18, 2018

Albeleon and others added some commits Jun 19, 2018

"2.3: RM2000 + RM2003: The battle command window (both left [manual, …
…automatic, escape] and right [attack, skill, defend, objects]) should move smoothly of position from one another instead of immediately (in RM2003, type of Window Battle Screen B and C)."
"2.5: RM2000: If the second line of a Skill is not empty: 1. The firs…
…t line should have a pause before the second line is printed, and 2. The animation should only start when the second line is printed."

Solution: We should print separate each line in a skill with a time waiting between them. For this:
1. We modify Game_BattleAlgorithm::Skill::GetStartMessage so it only prints the first line.
2. We create two functions in AlgorithmBase, IsSecondStartMessage (which checks if there is a second message pendent, by default false) and GetSecondStartMessage (which prints the second line, by default empty). We override them in Skill so it prints adequately.
3. In Scene_Battle_Rpg2k::ProcessBattleAction, we print only the first line if it's empty, and the second line if there is one element and IsSecondStartMessage is true.
4. For the First target, we check if the first line was printed and IsSecondStartMessage is true. In that case, we force it to pause until GetDelayForWindow time passes.
5. When the screen is cleared in BattleActionState_Result, it should print also the second line if it's available.
Objective: Implement the #1377 feature from RPG_RT:
"When a skill (with animation attached) is directed towards an ally target or ally group (enemy skill, or ally skill that heals), instead of playing the graphic animation, it plays only the sound. This is fine, but there is more than that: It only plays the sounds of the 20 first frames of the animation. If a sound is on frame 21 or further, it won't play. Also, the waiting time will always be around 15-20 frames, no matter its actual length (more or less)."

Solution:
 - Include a cutoff frame where, from the on, any Sfx operation (sound, flash or shake) isn't executed. We include this variable as an attribute in battle_animation and as an input argument for the constructor (mostly applied to BattleAnimationBattlers). Then we use then for any subsequent function made to invoke Battle Animations that (in our case) could summon SoundAnimation. Therefore, in Scene_Battle_Rpg2k.cpp, when it invokes that function, it puts 20 as the cutoff point.
 - Also, since the wait time for this animation doesn't matter, what we do is add a "ShouldOnlySound" function available for Scene_Battle_Rpg2k.cpp, so in case it's a sound, it doesn't have to wait until it's finishes, it just waits the normal GetDelayForWindow().
 - In that case, the wait after any animation is finished has been changed from GetDelayForLine() to GetDelayForWindow(), it's more exact.
"2.17: RM2000: When an attack heals states, that should be in battle_…
…algorithm, instead of being treated in the results. Also, it is possible for an attack to heal a state, and right after cause that state again."

Solution:
1. Move the function (Game_Battler::BattlePhysicalStateHeal) from Scene_Battle_Rpg2k to Battle_Algorithm::Execute, and returning the value to an attribute "healed_conditions".
2. Now BattlePhysicalStateHeal only return states, but doesn't remove them until later in Battle_Algorithm::Apply, so we take the RemoveStates.
3. Right after the HP damage, we print it. Also, since the result messages are printed before the states have been applied, we put in the condition result message "if it has a state and that state isn't included in healed_conditions, print it".
4. We apply the states removal before the new states are caused (again, in RPG_RT, if an attack causes sleep, you can wake up the enemy with the attack but right after put it to sleep again).

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:albeleon_battle_3 branch from 7ced8d5 to 9f90421 Nov 18, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 18, 2018

The 1 frame flicker bug turned out to be simple. So I just pushed a new commit fixing it.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 19, 2018

Are there anymore comments? Rebasing 5, 6, and 7 are blocked until this one goes in.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 19, 2018

I would give @carstene1ns a short grace period for feedback as I havn't seen him around since a week.
If theres no response will merge it tomorrow.
I will start to understand Battle 5 in the meanwhile. this is a huge blocker :/

@Ghabry Ghabry merged commit 204ee63 into EasyRPG:master Nov 20, 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
@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 20, 2018

merging this for now, in case of further feedback can be incorporated in the next PR, we have enough of them...

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