Skill - Synchronize

[JesterWizard]

Inflict the skill holder's status on the enemy.

As a side note, the number of turns is always set to 3 by default, even if the skill holder only has 1 turn left on their status. I wasn't sure whether to change that or not so the remaining turns is the same for both units.

[MokhaLeee]

Just try to answer three questions and you'll realize problem in your PR:

1. How does vanilla works on the familiar functionality, including Poison Axe/Nightmare/Stone and status staff? What functions are involved in the implementation of these item effects to give enemy debuff? How does each of these functions work?
2. The function you are hacking now, BattleUpdateBattleStats, what functionality does it works on in vanilla program?
3. What does function UpdateUnitFromBattle (which just the hacked location for skill Synchronize in community project) works on in vanilla program?

If you find it difficult to answer these questions, it is recommanded try to study on vanilla bmbattle related decomp source code.

I would like to give my point directly: Both the hacks in community project and this PR have failed to respect on the original program functionality, both of which modified in the wrong place, reducing the quality of the code and causing problems for other developers. Why it does not judge on hit or MISSING in community project is just because it cannot easily judge on it at the function it hacked.

I don't mind repeating my critique on current community project

The community project is just a mountain of shit filled with low-quality code without strict review.

This is understandable. In those days when decomp progress is less than 20% and FE-CLib was even not established, people didn't even understand how does it work on struct Unit. In an age when it was even hard to implement some features which seems very simple today, to be able to inherit so many features to build such a huge skill system is enough to be a remarkable achievement.

The boot is on the other foot.

At this point, we have a 97% complete decomp code, which allows us to make better changes.

I do not deny that the current modification in this PR is capable of fulfilling its goals, But at this moment, standing on the shoulders of giants, what we need to think about is not "whether it can be realized", but "whether it is the best solution to do so". That's exactly why we needed to rebuild a new C-skillsys and throw the old system out of the trash

[JesterWizard]

Okay taking what you've said into account, I've removed the temporary flag for the skill (because in hindsight it did fuck all). I've also changed the skill's position to inside BattleGenerateHitEffects and at the end of if (!(gBattleHitIterator->attributes & BATTLE_HIT_ATTR_MISS)) which will mean it'll override other status skill effects and weapn status effects.

[JesterWizard]

Synchronize shouldn't have to be a proc skill. It has enough restrictions to not make it necessary. There's strategic play to be had with sharing buffs to healers and debuffs to enemies, but you can't do that if you have to rely on luck for the skill to proc.

[MokhaLeee]

sorry but i failed to understand,why this routine should not lie in so called "proc skill"？

[JesterWizard]

I just explained why.

The skill requires a specific set of circumstances; that the user has a status and that the enemy doesn't. Furthermore it must be triggered during a battle/healing sequence. These conditions are restrictive enough. By making the skill require RNG on top of that, you're stripping it of any unique strategic play it might have, like granting a positive buff to a chain of healers, of debuffing multiple enemy units with a combination of the synchronize unit and a dancer unit.

In practice, this is now a dud skill with the RNG. It's not going to be overpowered if you remove the RNG so I don't see why it should have it.

[MokhaLeee]

Sounds like a gameplay issue, on considering on give allies positive status, it should not be triggered by RN excatly.

However, the combat action can only exec in battle such as attack to enemy or berserk attack. Not for heal or other staff effect. I'll look at it later, the hack on the staff or dance action needs additional treatment.

[MokhaLeee]

It seems that it should be a battle-hit skill, but post-acition