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

BattleFixes #826

Merged
merged 15 commits into from Mar 18, 2016

Conversation

Projects
None yet
2 participants
@Tondorian
Member

Tondorian commented Mar 17, 2016

#821

fixes #820
Items: Consuming SP
Items: crit
Items: Conditions
Items: Ignore Dogerate
Items: Attributes increase/decrease Damage
Defending/Charged: On Dead stoped
Interpreter: ConditionChange Dead reduces HP to 0 now
Spells: Attributes increase/decrease Damage
Conditions: Silence and Hit fixed

return false;
}
}

This comment has been minimized.

@Ghabry

Ghabry Mar 17, 2016

Member

inflicted, not inflected :)
Because the data type is simple to write you can also use "int" instead of "auto".

skill_id is off-by-one, needs a -1

if (weaponID != -1) {
source->SetSp(source->GetSp() - Data::items[weaponID].sp_cost);
}
}

This comment has been minimized.

@Ghabry

Ghabry Mar 17, 2016

Member

You can use ChangeSp here

std::vector<uint8_t> Game_Enemy::GetAttributeRanks() const {
return Data::enemies[GetId()-1].attribute_ranks;
}

This comment has been minimized.

@Ghabry

Ghabry Mar 17, 2016

Member

I see an off-by-one error in line 53. (needs a -1)

@@ -362,7 +362,7 @@ void Game_BattleAlgorithm::AlgorithmBase::Apply() {
if(source->GetType() == Game_Battler::Type_Ally) {
int weaponID = (static_cast<Game_Actor*>(source))->GetWeaponId() - 1;
if (weaponID != -1) {
source->SetSp(source->GetSp() - Data::items[weaponID].sp_cost);
source->ChangeSp(-Data::items[weaponID].sp_cost);

This comment has been minimized.

@Ghabry

Ghabry Mar 17, 2016

Member

This will consume SP even if the attack was not with the sword

@@ -518,6 +518,7 @@ bool Game_BattleAlgorithm::Normal::Execute() {
hit_chance = weapon.hit;
crit_chance = crit_chance += weapon.critical_hit;
multiplier = GetAttributeMultiplier(weapon.attribute_set);
source->ChangeSp(source->GetSp() - Data::items[weaponID].sp_cost);

This comment has been minimized.

@Ghabry

Ghabry Mar 17, 2016

Member

You make a permanent change in Execute()

if ((Data::states[inflictedState - 1].restrict_magic && Data::skills[skill_id].magical_rate > Data::states[inflictedState - 1].restrict_magic_level) || ((Data::states[inflictedState - 1].restrict_skill && Data::skills[skill_id-1].hit > Data::states[inflictedState - 1].restrict_skill_level))) {
return false;
}
}

This comment has been minimized.

@Ghabry

Ghabry Mar 17, 2016

Member

This is still of-by-one for skill_id, you use it twice (it's btw hard to find changes after reviewing when you completely rewrite the history^)

fixes index-shift
Execute is non Permanent again
Weapons consumes sp only if they are used
@@ -518,7 +512,7 @@ bool Game_BattleAlgorithm::Normal::Execute() {
hit_chance = weapon.hit;
crit_chance = crit_chance += weapon.critical_hit;
multiplier = GetAttributeMultiplier(weapon.attribute_set);
source->ChangeSp(source->GetSp() - Data::items[weaponID].sp_cost);
sp = - Data::items[weaponID].sp_cost;

This comment has been minimized.

@Ghabry

Ghabry Mar 18, 2016

Member

This will not work because the "sp = " value is for the SP change of the target.

@@ -764,7 +758,7 @@ void Game_BattleAlgorithm::Skill::Apply() {
}
else {
if (first_attack) {
source->SetSp(source->GetSp() - source->CalculateSkillCost(skill.ID));
source->ChangeSp(- source->CalculateSkillCost(skill.ID));

This comment has been minimized.

@Ghabry

Ghabry Mar 18, 2016

Member

You can do it like here in the first_attack-if instead (but in Normal::Apply ^^)

@Ghabry

This comment has been minimized.

Member

Ghabry commented Mar 18, 2016

Otherwise your PR looks okay. I can't test it currently but by just looking over it is fine.

I'm curious how magic weapons behave in RPG_RT, have to test this ^^

@@ -119,7 +119,7 @@ bool Game_Actor::IsSkillUsable(int skill_id) const {
return false;
}
for (int inflictedState : this->GetInflictedStates()) {
if ((Data::states[inflictedState - 1].restrict_magic && Data::skills[skill_id].magical_rate > Data::states[inflictedState - 1].restrict_magic_level) || ((Data::states[inflictedState - 1].restrict_skill && Data::skills[skill_id-1].hit > Data::states[inflictedState - 1].restrict_skill_level))) {
if ((Data::states[inflictedState - 1].restrict_magic && Data::skills[skill_id - 1].magical_rate > Data::states[inflictedState - 1].restrict_magic_level) || ((Data::states[inflictedState - 1].restrict_skill && Data::skills[skill_id-1].hit > Data::states[inflictedState - 1].restrict_skill_level))) {

This comment has been minimized.

@Ghabry

Ghabry Mar 18, 2016

Member

This code is already implemented in Game_Battler::IsSkillUsable. So I was wrong when I said that silence is not working yet. Sorry :(

This comment has been minimized.

@Tondorian

Tondorian Mar 18, 2016

Member

But i tested it befor i implement this and i was able to cast spells ..

This comment has been minimized.

@Ghabry

Ghabry Mar 18, 2016

Member

I didn't say that the code was wrong but that you should merge it with the restriction code of Game_Battler::IsSkillUsable ^^

@Tondorian

This comment has been minimized.

Member

Tondorian commented Mar 18, 2016

I tested it .. my version of rpg2k3 says: you will not be able to attack if sp is to low but tests shows it consumes sp to 0 and attacks allways works ...

@Tondorian Tondorian closed this Mar 18, 2016

@Tondorian Tondorian reopened this Mar 18, 2016

@Ghabry

This comment has been minimized.

Member

Ghabry commented Mar 18, 2016

Imo it would be very stupid if you could not attack normally anymore when SP are 0 so this makes sense ^^

@@ -193,6 +193,9 @@ int Game_Enemy::GetDropProbability() const {
}
bool Game_Enemy::IsActionValid(const RPG::EnemyAction& action) {
if(action.kind == action.Kind_skill) {
return IsSkillUsable(action.skill_id);
}
switch (action.condition_type) {

This comment has been minimized.

@Tondorian

Tondorian Mar 18, 2016

Member

Maybe this is not the best way to check
need feedback

@@ -61,6 +61,10 @@ int Game_Battler::GetSignificantRestriction() {
return RPG::State::Restriction_normal;
}
int Game_Battler::GetId() const {
return Game_Battler::GetId();

This comment has been minimized.

@Ghabry

Ghabry Mar 18, 2016

Member

warning C4717: "Game_Battler::GetId": Rekursiv für alle Steuerelementpfade. Die Funktion verursacht einen Stapelüberlauf zur Laufzeit.

if ((Data::states[inflictedState - 1].restrict_magic && Data::skills[skill_id - 1].magical_rate > Data::states[inflictedState - 1].restrict_magic_level) || ((Data::states[inflictedState - 1].restrict_skill && Data::skills[skill_id - 1].hit > Data::states[inflictedState - 1].restrict_skill_level))) {
return false;
}
}

This comment has been minimized.

@Ghabry

Ghabry Mar 18, 2016

Member

Please provide a test case where your code works. Because for me it fails.

This comment has been minimized.

@Tondorian

@Ghabry Ghabry added this to the 0.4.1 milestone Mar 18, 2016

fixes compiler warning
fixes IsSkillUsable bug
@Ghabry

This comment has been minimized.

Member

Ghabry commented Mar 18, 2016

1st row RPG_RT
2nd row old code
3rd row your new code

unbenannt

Reverting redundant silence check
sorry for stealing your time and delaying 0.4.1 :(

Ghabry added a commit that referenced this pull request Mar 18, 2016

@Ghabry Ghabry merged commit 1bc6244 into EasyRPG:master Mar 18, 2016

5 checks passed

Android Build finished.
Details
Linux Build finished.
Details
OSX Build finished.
Details
Windows Build finished.
Details
web Build finished.
Details

@Tondorian Tondorian deleted the Tondorian:feature/Battle branch Mar 23, 2016

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