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

Add skill to advanced defense #265

Merged
merged 5 commits into from Jun 18, 2020
Merged

Add skill to advanced defense #265

merged 5 commits into from Jun 18, 2020

Conversation

wershlak
Copy link
Contributor

@wershlak wershlak commented May 2, 2020

adding a skill to advanced defense for warriors and deikhans. This is very similar in concept to counter move for monks. Lets the tank classes have a chance to avoid some debilitating special moves.

@wershlak wershlak requested review from cizra and Aion2501 May 2, 2020 02:14
@wershlak wershlak requested a review from a team as a code owner May 2, 2020 02:14
@@ -223,6 +223,7 @@ static int bash(TBeing *c, TBeing *victim, spellNumT skill)
(bKnown > 0) &&
(i != GUARANTEED_FAILURE) &&
(!victim->canCounterMove(bKnown)) &&
(!victim->canFocusedAvoidance(bKnown/2)) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The () aren't necessary, but they don't hurt either.

code/code/cmd/cmd_bonebreak.cc Outdated Show resolved Hide resolved

if (victim->canFocusedAvoidance(bKnown/2)) {
SV(SKILL_BONEBREAK);
rc = bonebreakMiss(caster, victim, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a function call (or perhaps inside that function call), I'd just have an array of string triples. Look up the strings and print them — less code lines and no branch mispredictions in CPU.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not saying you should go and refactor everything in the world, though :)


CDDefense::CDDefense() :
CDiscipline(),
skAdvancedDefense()
skAdvancedDefense(),
skFocusedAvoidance()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chances are that these constructors, destructors, operator= don't do anything useful and should be deleted.

code/code/disc/disc_defense.cc Outdated Show resolved Hide resolved
@@ -1404,6 +1404,7 @@ class TBeing : public TThing {
bool checkSmashed(TBeing *, wearSlotT, spellNumT, TThing *, int, const char * = NULL);
int hit(TBeing *, int pulse = -1);
bool canCounterMove(int);
bool canFocusedAvoidance(int);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why these skills live in TBeing? This causes horrible recompilation times for everything, as well as leaking inner workings of disciplines into generic code of beings.

As most things in TBeing are public, maybe it makes more sense to have that code in disciplines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like all the combat type code is dealing with two objects that are subclasses of TBeing of some sort. So it kind of makes sense to be able to write

if (victim->canFocusedAvoidance(bKnown/2)) {

I guess the other option would just be

if (canFocusedAvoidance(victim, bKnown/2)) {

Just not object oriented. It seems a little easier to reason about in the first instance but if the performance impact is huge the other way it's not a big deal to change it all.

I feel like "Disciplines" or "Abilities" maybe should be a class that is instantiated per being.

code/code/misc/skills.cc Show resolved Hide resolved
@cizra cizra merged commit c381640 into master Jun 18, 2020
@cizra cizra deleted the adv_def branch June 18, 2020 08:59
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

2 participants