-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Core/Scripts: Gnomish Mind Control Cap and Gnomish Universal Remote #17374
Conversation
class spell_item_universal_remote_SpellScript : public SpellScript | ||
{ | ||
PrepareSpellScript(spell_item_universal_remote_SpellScript); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load should also be overrided here with the same check of spell_item_mind_control_cap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so you mean it should look like this:
class spell_item_universal_remote_SpellScript : public SpellScript
{
PrepareSpellScript(spell_item_universal_remote_SpellScript);
bool Load() override
{
if (!GetCastItem())
return false;
return true;
}
bool Validate(SpellInfo const* /*spellInfo*/) override
{
Weird that joschiwald did not add it, he wrote the whole section below
// 8344 - Universal Remote (Gnomish Universal Remote)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's what I meant. You can wait for the original author if you feel like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it, if you are as sure about this change as joschiwald is in his comments. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The probabilities of this being casted by a non-item is really low, but not 0. Joschiwald was focusing the roll chance management/spellinfo validation code with that comment.
If you want more insight about this, debug the castspell call and see if nullptr item is handled somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useless
can you update the first spellscript similar to the second one? |
}); | ||
|
||
if (Unit* target = GetHitUnit()) | ||
GetCaster()->CastSpell(target, itr->second, true, GetCastItem()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
itr->second
is double
, you probably mean itr->first
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Remember, this is elecyb's code, so I do what you suggest. :-)
Sure, I will do my best to make them similar in structure. |
Going to rebase this PR soon to fix those conflicts. Been occupied with other things, but will fix this soon. |
return true; | ||
} | ||
|
||
void HandleDummy(SpellEffIndex /*effIndex*/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be more fitting for the purpose:
if (Unit* target = GetHitUnit())
{
uint8 chance = urand(0, 99);
if (chance < 15)
GetCaster()->CastSpell(target, SPELL_TARGET_LOCK, true, GetCastItem());
else if (chance < 35)
GetCaster()->CastSpell(target, SPELL_MOBILITY_MALFUNCTION, true, GetCastItem());
else
GetCaster()->CastSpell(target, SPELL_CONTROL_MACHINE, true, GetCastItem());
}
Instead of the unordered_map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is more redable btw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so that means I can remove
static std::unordered_map<uint32 /*spellId*/, double /*probability*/> const UniversalRemoteActions =
{
{ SPELL_CONTROL_MACHINE, 60.0 },
{ SPELL_MOBILITY_MALFUNCTION, 25.0 },
{ SPELL_TARGET_LOCK, 15.0 }
};
and replace that with enum instead. I will take a look at it as soon as I have done some other work.
enum UniversalRemote
{
CHANCE_TARGET_LOCK = 15,
CHANCE_MOBILITY_MALFUNCTION = 25,
SPELL_CONTROL_MACHINE = 8345,
SPELL_MOBILITY_MALFUNCTION = 8346,
SPELL_TARGET_LOCK = 8347
};
if (Unit* target = GetHitUnit())
{
uint8 chance = urand(1, 100);
if (chance <= CHANCE_TARGET_LOCK)
GetCaster()->CastSpell(target, SPELL_TARGET_LOCK, true, GetCastItem());
else if (chance <= CHANCE_MOBILITY_MALFUNCTION)
GetCaster()->CastSpell(target, SPELL_MOBILITY_MALFUNCTION, true, GetCastItem());
else
GetCaster()->CastSpell(target, SPELL_CONTROL_MACHINE, true, GetCastItem());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm wrong but the % will change with this code
SPELL_TARGET_LOCK [1, 15] - 15%
SPELL_MOBILITY_MALFUNCTION [16, 25] - 10%
SPELL_CONTROL_MACHINE [26, 100] - 75%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitVerge is right. SPELL_MOBILITY_MALFUNCTION should be changed to 35 instead of 25 :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct number is 40 (15+25) and the other 60 for control machine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, maybe you're right. I don't know the calculation well enough to tell myself.
(mostly because I overtook the PR from another user to make it completable)
Implement scripts for Mind Amplification Dish and Gnomish Universal Remote Adds functionality to the following items: - Mind Amplification Dish (enchant) - Gnomish Thinking Cap - Gnomish Mind Control Cap - Gnomish Universal Remote By elecyb & joschiwald Closes #2562
Changes proposed:
Implement scripts for Mind Amplification Dish and Gnomish Universal Remote.
Based on the previous PR #16232 by @elecyb and feedback in that PR.
Adds functionality to the following items:
Chances are based on empirical results from wowhead (highly upvoted post):
http://www.wowhead.com/spell=67839/mind-amplification-dish#comments:id=1483300
By elecyb & joschiwald
Target branch(es): 3.3.5 / 6.x
Issues addressed: Closes #2562
Tests performed: (Does it build, tested in-game, etc) Builds without issues.
Known issues and TODO list: