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

Battle 2k3: Add support for flipped battle animations #2310

Merged
merged 4 commits into from Sep 9, 2020
Merged

Battle 2k3: Add support for flipped battle animations #2310

merged 4 commits into from Sep 9, 2020

Conversation

ghost
Copy link

@ghost ghost commented Aug 30, 2020

This PR takes on a more difficult task and fixes #1564. The changes look more intrusive so it is possible that changes are required on this PR. Anyway, this basically adds a optional invert parameter on the BattleAnimationBattle constructor, the ShowBattleAnimation method and the PlayAnimation methods which makes the animations display mirrored if the parameter is set to true. The new parameter gets used in Scene_Battle_Rpg2k3 only because this feature is used in RPG Maker 2003 only. Some particularities about the flipped animations: Animations get flipped only if the "Flip assets if facing the other direction" flag is set. If this condition is met then the animations will be flipped if either the battler sprite of the source is flipped or the source is belonging to the enemy party (XOR-Operation). On reflected spells the second animation will be flipped if either the battler sprite of the original target is flipped or the original target is belonging to the enemy party (XOR-Operation).

Flipped battle animations are supported now.
@@ -815,6 +823,14 @@ bool Game_BattleAlgorithm::AlgorithmBase::OriginalTargetsSet() const {
return (original_targets.size() > 0);
}

Game_Battler* Game_BattleAlgorithm::AlgorithmBase::GetOriginalTarget() const {
if (current_original_target == original_targets.end()) {
return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

nullptr

@@ -574,3 +574,7 @@ void Game_System::IncFrameCounter() {
Color Game_System::GetBackgroundColor() {
return bg_color;
}

const bool& Game_System::GetInvertAnimations() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Primitive types like ints and bools should be returned by value, not const ref. Generally references are less efficient for anything less than or equal to 2 words which is also trivially copyable.

if (Game_System::GetInvertAnimations()) {
invert_animation = false;
if (action->OriginalTargetsSet()) {
invert_animation = (action->GetOriginalTarget()->IsDirectionFlipped() ^ (action->GetOriginalTarget()->GetType() == Game_Battler::Type_Enemy));
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this xor logic behind an inline function so it's not copy pasted. Give it a meaningful name so we know what it's trying to do without needing comments.

It can be a member of function of Game_Battler or a free function in scene_battle_rpg2k3 which takes a Game_Battler as a parameter. Make sure it's inlined so we don't pay the cost of a function call just to do an xor.

@@ -924,11 +932,17 @@ bool Scene_Battle_Rpg2k3::ProcessBattleAction(Game_BattleAlgorithm::AlgorithmBas
Sprite_Battler::LoopState_WaitAfterFinish);
}

if (Game_System::GetInvertAnimations()) {
invert_animation = (action->GetSource()->IsDirectionFlipped() ^ (action->GetSource()->GetType() == Game_Battler::Type_Enemy));
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing state in the scene adds complexity, makes the code harder to follow, and makes it easier to introduce bugs. Instead, I would suggest re-computing this boolean flag directly using the source and target in the other place, instead of storing and retrieving this invert_animation flag.

@fmatthew5876
Copy link
Contributor

On reflected spells the second animation will be flipped if either the battler sprite of the original target is flipped or the original target is belonging to the enemy party

What if the spell is targeting the enemy party and some but not all enemies are flipped? Which enemy is used to determine if the animation will flip?

@@ -108,8 +108,10 @@ void BattleAnimation::DrawAt(Bitmap& dst, int x, int y) {
}

SetX(cell.x + x);
if (invert) SetX(x - cell.x);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor performance nit: You're calling SetX() twice here. Sprite::SetX() is currently inlined so right now it doesn't matter, but later if SetX() ever changed to be out of line this would be slower.

Lets make it a little more future proof by computing the new x once and then calling SetX(). You could use a ternary operator or create a temporary variable and assign it.

anim_targets,
false,
-1,
invert);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't have to be done in this PR, but when we get this level of mess it's time to convert these defaulted parameters to a BattleAnimationArgs struct.

@@ -766,6 +773,7 @@ void Game_BattleAlgorithm::AlgorithmBase::TargetFirst() {
} else {
if (IsReflected()) {
original_targets.push_back(GetTarget());
current_original_target = original_targets.begin();
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid storing additional member variables unless you really need them. More state means more complex and hard to follow code.

Can't you change GetOriginalTarget() to GetFirstOriginalTarget() and just return the first one directly? Or change it to GetOriginalTargets() and return a reference to the whole vector and let the caller decide what to do with it.

The CheckAnimFlip function has been added which takes a Game_Battler
as a parameter. This puts the whole XOR-Operations in one place.
Moreover the invert_animation flag has been removed because the
changes made storing the state unnecessary. And it is sufficient
to check the first original target on a reflected animation.
@ghost
Copy link
Author

ghost commented Aug 31, 2020

I have added a commit now which tries to take care of the issues you mentioned. I have added a CheckAnimFlip function which puts the XOR-Operation in one place. I hope I didn't miss anything important.

@@ -815,6 +816,10 @@ bool Game_BattleAlgorithm::AlgorithmBase::OriginalTargetsSet() const {
return (original_targets.size() > 0);
}

Game_Battler* Game_BattleAlgorithm::AlgorithmBase::GetFirstOriginalTarget() const {
return *original_targets.begin();
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're going to return a pointer, check for empty first and return null if no original targets. Otherwise, return a Game_Battler& and add an assert(!original_targets.empty())

Copy link
Author

Choose a reason for hiding this comment

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

This is probably a dumb question but should I still make the change what this function returns? Because you have successfully made tests and I'm afraid I will break something if I try to change this. And maybe I missed something but I don't quite understand the assert suggestion. For me it looks like an assertion gets triggered when original targets are set.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very minor change, and converting pointer to reference any bugs will be compiler errors.

Just be sure to get the assert correct. assert(!original_targets.empty()) means crash the program if I call this function and the targets are empty. asserts are only enabled in debug mode so you can quickly test a debug build.

Copy link
Member

Choose a reason for hiding this comment

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

Currently your code will crash when there are no original_targets. This means you make an assumption here that this is not supposed to happen. An assert is nice to mark impossible situations - When the assert is triggered it means the code has a bug. (note that user input should never trigger asserts, for user generated problems there should be more sane error handling)

Using & instead of * here makes sense because this indicates to the caller "The return value is always a valid Battler". For * the caller must expect NULL/nullptr.

Copy link
Author

Choose a reason for hiding this comment

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

I somehow could not think straight for the last few hours. But now I went for the first variant in @fmatthew5876 's comment, returning a pointer and doing the check for empty. I have pushed a new commit.

Copy link
Contributor

@fmatthew5876 fmatthew5876 left a comment

Choose a reason for hiding this comment

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

I tested this with normal, back attack, surround, and pincer and it works! The code is also very clean. Great job @rueter37 , this was something in the backlog for a long time and I'm really happy someone was finally able to take it on.

Other than my last minor comment, this is good to go from my perspective.

@Ghabry Ghabry added this to the 0.6.3 milestone Sep 2, 2020
If the list of original targets is empty, this function returns NULL.
@fmatthew5876
Copy link
Contributor

Use nullptr instead of NULL. After that, this is ready to be merged!

The GetFirstOriginalTarget function now returns nullptr if the
original_targets list is empty.
@ghost
Copy link
Author

ghost commented Sep 2, 2020

Added a new commit which contains the change you requested.

@fmatthew5876
Copy link
Contributor

I'm ok to merge this anytime.

Thank you for your contribution

@Ghabry Ghabry merged commit baaa79a into EasyRPG:master Sep 9, 2020
@ghost ghost deleted the issue-1564 branch September 9, 2020 15:23
@Ghabry
Copy link
Member

Ghabry commented Sep 10, 2020

scene_battle_rpg2k3.cpp:1489:40: warning: ^ has lower precedence than ==; == will be evaluated first [-Wparentheses]
                return battler->IsDirectionFlipped() ^ battler->GetType() == Game_Battler::Type_Enemy;

Well this is what we want, but warning should be silenced by someone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Battle 2k3: Flip assets if facing the other direction option
3 participants