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: Battle animation sprites changes #2482

Merged
merged 6 commits into from Apr 17, 2021
Merged

Battle 2k3: Battle animation sprites changes #2482

merged 6 commits into from Apr 17, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 15, 2021

This PR intends to do the following fixes:

  • Placement of battlers: The battlers were placed wrong (too much distance between each other and partially and full off-screen placing starting with the third party member) if battle animation sprites were used. BattleCharset sprites are working fine.
  • Displaying state afflictions: While BattleCharset sprites are showing its state afflictions at the beginning of the battle, the battle animation sprite battlers did so not until their first turn.
  • Battle animation sprites are flipped now if the flip flag is set.
  • BattleAnimation height: Battle animations are displayed on the right height if the target is a battle animation sprite.

@Ghabry Ghabry added Battle Has PR Dependencies This PR depends on another PR labels Mar 15, 2021
@ghost ghost mentioned this pull request Mar 15, 2021
91 tasks
@Ghabry Ghabry added Awaiting Rebase Pull requests with conflicting files due to former merge and removed Has PR Dependencies This PR depends on another PR labels Mar 21, 2021
@Ghabry Ghabry added this to the 0.6.3 milestone Mar 21, 2021
@ghost ghost marked this pull request as ready for review March 22, 2021 04:55
@fdelapena fdelapena added Animations and removed Awaiting Rebase Pull requests with conflicting files due to former merge labels Mar 22, 2021
}
return Sprite::GetWidth();
}

int Sprite_Actor::GetHeight() const {
if (animation) {
return animation->GetHeight();
return animation->GetAnimationCellHeight() / 2;
Copy link
Member

Choose a reason for hiding this comment

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

RPG Maker 2003 has two types of battle animations (small and big), did you check this?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the pointer, will make some tests tomorrow with the big sprites. The small ones should work fine.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks again for checking on this one, the big sprites were indeed broken. GetAnimationCellWidth() and GetAnimationCellHeight() always returned 96 (which is correct for normal sprites) but must return 128 on big sprites.

rueter37 added 4 commits March 24, 2021 18:26
If battle animations are used for the actors, assume half of the
battle animation height and width for the height and width values.
If a character uses a battle animation sprite and is affected by a
state, the battle sprite showed its state affliction not until after
its first turn. This commit aims to fix that.
The battle animation battlers are now able to be flipped too in non-
normal or initiative battle conditions.
The battle animations use the correct height now if the target uses
a battle animation sprite.
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

if (sprite->GetBitmap()) {
offset = CalculateOffset(animation.position, sprite->GetHeight());
} else {
offset = CalculateOffset(animation.position, GetAnimationCellHeight() / 2);
Copy link
Member

Choose a reason for hiding this comment

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

just a marker for me to test this in emscripten

Copy link
Member

Choose a reason for hiding this comment

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

well yeah this works. As this depends on the battle sprite the sprite is usually already loaded before anyway, so no emscripten worries here

if (sprite) {
sprite->ResetZ();
sprite->DetectStateChange();
sprite->RefreshZ();
Copy link
Member

Choose a reason for hiding this comment

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

can you put the code of RefreshZ in ResetZ or is there a reason why this doesnt work?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately the code cannot be put into ResetZ, because ResetZ is in sprite_battler while the code has to be run inside of sprite_actor because of the animation variable.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest that you simply make ResetZ a virtual function and then you override it in this class. Then the RefreshZ is not needed.
(To call the parent of an overridden method use Sprite_Battler::ResetZ())

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

ResetZ() is called by the constructors of Sprite_Actor() and Sprite_Enemy(). This means that if someone even subclassed them and tried to override it, the child class function would not get called as the child class hasn't been constructed yet.

If we're going to do this, lets make ResetZ() final in both of these classes to prevent overriding it.

Copy link
Author

Choose a reason for hiding this comment

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

Added a commit now which disallows overriding the ResetZ() function in sprite_actor and sprite_enemy.

Copy link
Member

Choose a reason for hiding this comment

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

My bad forgot to check for this annoying limitation :/

src/battle_animation.cpp Show resolved Hide resolved
src/battle_animation.cpp Outdated Show resolved Hide resolved
for (auto& battler: battlers) {
SetFlashEffect(battler->GetFlashColor());
}
Sprite::Draw(dst);
Copy link
Member

Choose a reason for hiding this comment

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

Is this copy-paste here really necessary?
You are already overloading FlashTargets can you tell us what is not working here making this necessary? Then we can work on a fix.

Copy link
Author

Choose a reason for hiding this comment

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

The copy-pasted code is unfortunately needed for making the flash effects on battle animation sprites work.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the flash and shake stuff now (see below).

@Ghabry
Copy link
Member

Ghabry commented Mar 29, 2021

When I test with a battle animation that has a shake RPG_RT ignores them for both Target and Screen but in Player the shake always happens.

Also the flash does not work properly:

  • When Flash is set to "Target" RPG_RT does nothing
  • When Set to "Screen" the Actor flashes (wtf)

What does "BattleCharset sprite fully work here" mean? Is is even possible for them to Flash or Shake?

@ghost
Copy link
Author

ghost commented Mar 30, 2021

I have removed the flash and shake changes now, because the were broken anyway. Fixing the flash and shake stuff will be handled in another PR.

@Ghabry
Copy link
Member

Ghabry commented Mar 30, 2021

okay then this PR is basicly ready except for the emscripten test I want to do 👍

rueter37 added 2 commits March 30, 2021 19:47
The Z-Index is working correctly now on battle animations sprites on
battle start.
The ResetZ function in the sprite_actor and sprite_enemy classes can
no longer be overridden.
@ghost
Copy link
Author

ghost commented Apr 1, 2021

@Ghabry Despite being outside of the scope of this PR, I have to add something about the flashes:

When Set to "Screen" the Actor flashes (wtf)

This is already broken in master. You cannot reproduce it using a battle animation sprite, because flashes are not working there at all, but on enemies. Create a battle animation which uses screen flashes only and watch the enemies upon usage of the battle animation. You will see the enemies flash once in the flash color set in the battle animation settings despite the scope being set to "Screen".

Forget this one, I must have seen it wrong.

@ghost
Copy link
Author

ghost commented Apr 2, 2021

@Ghabry I forgot to answer to this comment from you: The problems you mentioned are now handled by #2505. And BattleCharset sprites are affected by flash and shake effects which is already working (this is meant with "BattleCharset sprites fully work here").

@Ghabry Ghabry requested a review from fmatthew5876 April 4, 2021 15:37
@Ghabry
Copy link
Member

Ghabry commented Apr 12, 2021

Just a reminder that, when there is no further feedback, this will be merged Sunday (6 days, 2 weeks after last review).

@fmatthew5876 fmatthew5876 merged commit 2010b2c into EasyRPG:master Apr 17, 2021
@ghost ghost deleted the battleanimation-sprites branch April 17, 2021 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants