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

Fixed caster damage modifier to use appropriate PvP modifier instead of PvM #2548

Merged
merged 2 commits into from
Dec 18, 2019

Conversation

dgarson
Copy link
Contributor

@dgarson dgarson commented Dec 18, 2019

It appears that the casting damage modifier being used for PvP mode is in fact unaffected by the PvP modifier for weapon damage, so it is using the PvM caster modifier instead. This patch includes the modifier which is the piece that includes reduction of the caster modifier for PK purposes.

@gmriggs gmriggs merged commit d515d3d into ACEmulator:master Dec 18, 2019
@harliq
Copy link
Collaborator

harliq commented Dec 18, 2019

On a side note, while at it, is there any reason for the ElementalDamageBonusPvPReduction to be a static readonly in the class instead of a local var in that function? Its only used in that function. This has nothing to do with what dgarson submitted, but a quick refactor would remove a couple of vars

@gmriggs
Copy link
Collaborator

gmriggs commented Dec 18, 2019

I dunno, whoever wrote that code defined a lot of odd variables -- like 'defaultModifier 1.0', 'defaultBonus 0.0', things that will absolutely never be changed, but for some reason there are just named vars defined for them

What you are saying is true from a scoping perspective, but unfortunately C# does not support static vars being declared from within a function. If a regular elementalDamageBonusPvPReduction = 0.5f were to be defined in that function, then it would be redundantly re-declaring it each and every time that function would be called

@harliq
Copy link
Collaborator

harliq commented Dec 18, 2019

I guess the question would be does it even need to be defined? If it's always the same, no real reason to define it, unless someone wanted to make it server configurable.

@gmriggs
Copy link
Collaborator

gmriggs commented Dec 18, 2019

Yes, that would be the question

At one point, i started to remove a lot of those 'defaultModifier' and 'defaultBonus' references and justmake them 1.0 / 0.0, but I found a lot of cases it did improve the readability of the code somewhat

So as it stands now, this really should be a completely non-issue

This is literally using 4 bytes of extra memory across the entire server..

@harliq
Copy link
Collaborator

harliq commented Dec 18, 2019

Agreed it's not an issue, just more of my own curiosity.

@gmriggs
Copy link
Collaborator

gmriggs commented Dec 18, 2019

"If it's always the same"

it's always the same, until someone wants to modify it. bring pvp down from 50%->25%, in a custom mod, for example... the client display wouldn't match up, but someone is currently free to modify that var from a named var, instead of twiddling the constants in the formula

also just because it's only referenced from within that function today, does not mean it could not be referenced from elsewhere in the future

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.

None yet

3 participants