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

[Monk] Mistweaver implementation #9368

Open
wants to merge 39 commits into
base: thewarwithin
Choose a base branch
from

Conversation

acornellier
Copy link

No description provided.

@acornellier acornellier marked this pull request as ready for review August 30, 2024 04:30
engine/class_modules/monk/sc_monk.cpp Outdated Show resolved Hide resolved
// The Ancient Concordance buff's spell data modified number of targets hit, but the value is 1 instead of 3, so it
// is useless. We set the value manually to 3.
if ( p()->buff.ancient_concordance->check() )
return 3;
Copy link
Member

Choose a reason for hiding this comment

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

It's not necessarily useless, as it may provide 2 stacks in game. Note that the effect has the "Aura Points on Client" bit set, which has to do with behaviours related to effect stacking.

Would need to perform some in game science, as it is also not logged.

else if ( baseline.mistweaver.teachings_of_the_monastery->ok() )
shared.teachings_of_the_monastery = baseline.mistweaver.teachings_of_the_monastery;
else
shared.teachings_of_the_monastery = spell_data_t::not_found();
Copy link
Member

Choose a reason for hiding this comment

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

If you're not comfortable writing a new equivalent function to _priority that is more friendly to multiple parameter types, let me know and I can put something together.

This feels gross, even I'm generally trying to get away from shared spell pointers.

Copy link
Author

Choose a reason for hiding this comment

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

sure, no idea how to rewrite _priority tbh

@acornellier acornellier force-pushed the mw branch 5 times, most recently from a65ac9c to b2a0efc Compare September 4, 2024 22:51
@acornellier acornellier changed the title [Monk] Mistweaver tiger palm, blackout kick [Monk] Mistweaver implementation Sep 5, 2024
@renanthera
Copy link
Member

renanthera commented Sep 9, 2024

The only things left to double check are TP OF working (I'm unsure if that's resolved) and whether or not all of the appropriate actions are using SP conversion properly when MW.

At that point, I would consider this good to merge (even prior to SEF completion).

A more flexible _priority is certainly a TODO, but not mandatory. It's in the same category of priority as my intended spell data storage changes.

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.

2 participants