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

Refactor move animations #4683

Merged
merged 15 commits into from
Jun 2, 2024

Conversation

cawtds
Copy link

@cawtds cawtds commented May 31, 2024

Description

Move animations are currently accessed via one large array, which needs to be sorted according to move ids. Ordering errors can easily go unnoticed due to the size of the array.
This PR removes the array and adds pointers to the animation scripts in gMovesInfo instead.

Discord contact info

.cawt

@AsparagusEduardo
Copy link
Collaborator

Something to test: What happens when battleAnimScript is unset.

@cawtds
Copy link
Author

cawtds commented Jun 1, 2024

Something to test: What happens when battleAnimScript is unset.

Game freezes/gets stuck

src/battle_anim.c Outdated Show resolved Hide resolved
@AlexOn1ine
Copy link
Collaborator

@AsparagusEduardo and @ghoulslash any more concerns?

@AsparagusEduardo
Copy link
Collaborator

any more concerns?

Two mainly:

  • This is targetting master.
  • Has the migration script been tested with someone's custom animations?

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Jun 1, 2024

Has the migration script been tested with someone's custom animations?

I was planning to test it with a custom move. It would be better if we asked someone with a lot of new moves though. Could someone ask Duke (I know they have a lot of new moves) or I do it later?

@AlexOn1ine
Copy link
Collaborator

This is targetting master

@cawtds could you retarget the pr to upcoming? Our workflow is that bugfixes go to master and new stuff to upcoming.

@cawtds cawtds changed the base branch from master to upcoming June 1, 2024 13:57
@AlexOn1ine
Copy link
Collaborator

Sorry @cawtds. One last thing. Could you add to the script to add this piece in moves_info.h:

#include "battle_anim_scripts.h"

@cawtds
Copy link
Author

cawtds commented Jun 1, 2024

Sorry @cawtds. One last thing. Could you add to the script to add this piece in moves_info.h:

#include "battle_anim_scripts.h"

✔️

@AlexOn1ine
Copy link
Collaborator

Sorry... One more thing I'll promise to merge after it. 🙏

It seems const u8 *battleAnimScript isn't added to the move struct.

@cawtds
Copy link
Author

cawtds commented Jun 1, 2024

Sorry... One more thing I'll promise to merge after it. 🙏

It seems const u8 *battleAnimScript isn't added to the move struct.

Not sure what you mean exactly. As part of the migration script or in general?

@AlexOn1ine
Copy link
Collaborator

Sorry... One more thing I'll promise to merge after it. 🙏
It seems const u8 *battleAnimScript isn't added to the move struct.

Not sure what you mean exactly. As part of the migration script or in general?

in the migration script. That one is missing there

@AsparagusEduardo
Copy link
Collaborator

AsparagusEduardo commented Jun 1, 2024

Regarding adding #include "battle_anim_scripts.h" and const u8 *battleAnimScript via the migration script, I was wondering if it's actually needed, since those additions would be handled with the update merge done by the user (as far as I know). The only scenario I could think of is if they manually remove both when doing the merge.

@AlexOn1ine
Copy link
Collaborator

Regarding adding #include "battle_anim_scripts.h" and const u8 *battleAnimScript via the migration script, I was wondering if it's actually needed, since those additions would be handled with the update merge done by the user (as far as I know). The only scenario I could think of is if they manually remove both when doing the merge.

Yeah, that make sense. My bad.

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Jun 1, 2024

Regarding adding #include "battle_anim_scripts.h" and const u8 *battleAnimScript via the migration script, I was wondering if it's actually needed, since those additions would be handled with the update merge done by the user (as far as I know). The only scenario I could think of is if they manually remove both when doing the merge.

Yeah, that make sense. My bad.

Actually they would probably make it worse since it would be a duplicate.
My bad @cawtds could you please revert the last change I asked for? Was my bad.

@cawtds
Copy link
Author

cawtds commented Jun 1, 2024

Regarding adding #include "battle_anim_scripts.h" and const u8 *battleAnimScript via the migration script, I was wondering if it's actually needed, since those additions would be handled with the update merge done by the user (as far as I know). The only scenario I could think of is if they manually remove both when doing the merge.

Yeah, that make sense. My bad.

Actually they would probably make it worse since it would be a duplicate. My bad @cawtds could you please revert the last change I asked for? Was my bad.

😢

@AsparagusEduardo
Copy link
Collaborator

Actually they would probably make it worse since it would be a duplicate. My bad @cawtds could you please revert the last change I asked for? Was my bad.

😢

Yeah, sorry about that, we shouldn't ping-ponging collaborators like this 😔

@cawtds
Copy link
Author

cawtds commented Jun 1, 2024

Actually they would probably make it worse since it would be a duplicate. My bad @cawtds could you please revert the last change I asked for? Was my bad.

😢

Yeah, sorry about that, we shouldn't ping-ponging collaborators like this 😔

no worries, just saw it like 1 sec too late

@AsparagusEduardo
Copy link
Collaborator

AsparagusEduardo commented Jun 1, 2024

Oh, before anyone merges, #4681 seems to have sneaked in because this PR it was based off master.

I have to go now, so someone please merge master into upcoming so that @cawtds can merge with upcoming and avoid having that PR's changes on this one.

@AsparagusEduardo
Copy link
Collaborator

Quickly came back to update upcoming with master, so just fixing conflicts should be needed before merge.

@AlexOn1ine AlexOn1ine merged commit 97143e0 into rh-hideout:upcoming Jun 2, 2024
1 check passed
@cawtds cawtds deleted the refactor-move-animations branch August 12, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants