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

updated items.xml with initdamage #3098

Merged
merged 3 commits into from Aug 1, 2020
Merged

updated items.xml with initdamage #3098

merged 3 commits into from Aug 1, 2020

Conversation

EPuncker
Copy link
Contributor

follow up of f58ea81

fields were not dealing damage on stepIn:
rme

@EPuncker EPuncker added the needs-testing Needs testing with screenshots label Jun 17, 2020
@EPuncker EPuncker requested a review from nekiro June 17, 2020 12:50
@Znote Znote added this to the 1.4 milestone Jun 17, 2020
@Znote
Copy link
Member

Znote commented Jun 17, 2020

The previous commit shouldn't have been merged into 1.4, it changes datapack behavior.
We might want to consider "damage" as "initdamage" for fields that inflict damageconditions.
@nekiro @ninjalulz

@nekiro
Copy link
Member

nekiro commented Jun 17, 2020

@Znote do we really need to be so strict with backwards compatibility? This is simple xml change it is not used in any script or anything.

nekiro
nekiro previously approved these changes Jul 2, 2020
@DSpeichert
Copy link
Member

Yes

@nekiro
Copy link
Member

nekiro commented Jul 6, 2020

Well, so right now. There is backwards incompatible field fix in this repo, revert it or merge this.

@Znote
Copy link
Member

Znote commented Jul 8, 2020

Well, so right now. There is backwards incompatible field fix in this repo, revert it or merge this.

Can you commit a fix for backwards compatibility?
If this is a field/condition, see if there is an initdamage, if not, see if there is damage and use that as initdamage.
If the initdamage is configured, the above check isn't neccesary and you can go ahead with initdamage.

I might try to look into it later, but since you made this it would be great if you could fix this. I do like your commit, we just gotta keep it versatile. :)

@nekiro
Copy link
Member

nekiro commented Jul 8, 2020

Well, so right now. There is backwards incompatible field fix in this repo, revert it or merge this.

Can you commit a fix for backwards compatibility?
If this is a field/condition, see if there is an initdamage, if not, see if there is damage and use that as initdamage.
If the initdamage is configured, the above check isn't neccesary and you can go ahead with initdamage.

I might try to look into it later, but since you made this it would be great if you could fix this. I do like your commit, we just gotta keep it versatile. :)

There is a reason I named it initdamage and not damage. Damage is reserved and used for field damage which is issued by condition. Initdamage is not pushed into condition damage list, so to make it backwards compatible you would have to do some dirty hacks. If you find a way you can tell me.

forgottenserver/src/items.cpp

Lines 1211 to 1225 in f58ea81

} else if (tmpStrValue == "damage") {
int32_t damage = -pugi::cast<int32_t>(subValueAttribute.value());
if (start > 0) {
std::list<int32_t> damageList;
ConditionDamage::generateDamageList(damage, start, damageList);
for (int32_t damageValue : damageList) {
conditionDamage->addDamage(1, ticks, -damageValue);
}
start = 0;
} else {
conditionDamage->addDamage(count, ticks, damage);
}
}
}

Znote added a commit to Znote/forgottenserver that referenced this pull request Jul 9, 2020
If a field item is missing initdamage property, load damage as initdamage.
This is to retain backward compatibility after otland#2815 which was addressed in otland#3098
@Znote
Copy link
Member

Znote commented Jul 10, 2020

#3128 should be merged before this, and this PR can then remove any additions where initdamage is identical to ordinary damage. (and only include changes where initDamage is different to damage).

@EPuncker
Copy link
Contributor Author

#3128 should be merged before this, and this PR can then remove any additions where initdamage is identical to ordinary damage. (and only include changes where initDamage is different to damage).

done

EPuncker pushed a commit that referenced this pull request Aug 1, 2020
* Use damage instead of initDamage if not defined
If a field item is missing initdamage property, load damage as initdamage.
This is to retain backward compatibility after #2815 which was addressed in #3098
@Znote Znote merged commit f0e2391 into otland:master Aug 1, 2020
@EPuncker EPuncker deleted the damageinit branch August 1, 2020 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-testing Needs testing with screenshots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants