Simplify script naming and implement AutoScript class

[boxa]

Full backward compatibility, old code will work without any changes.
This just a wrapper for an Script object, simplifies the code.

Usage examples:

AutoScript("script_name")->GetAI = &GetAI_realization;

**
AutoScript s("script_name");
s->pGossipHello = &GossipHello_realization;
s->pGossipSelect = &GossipSelect_realization;

**
AutoScript s;
s.newScript("script_name_1")->GetAI = &GetAI_realization_1;
s.newScript("script_name_2")->GetAI = &GetAI_realization_2;

**
AutoScript s;

s.newScript("script_name_1");
s->GetAI = &GetAI_script_name_1;

s.newScript("script_name_2");
s->GetAI = &GetAI_script_name_2;

[technoir42]

I think the class name 'AutoScript' doesn't match the purpose of the class and can be confusing. AutoScript isn't a script at all.

[Lillecarl]

It's not a script. But it refers to a script?

[DasBlub]

what i don't like about the patch is that he mixes this functionality with other changes like the intentions in ScriptMgr.h (the change is good but should not be in the same commit!). please don't add them with this PR but create another commit for that.

also, about the usage. I don't think that there's any need to write the code like this:

s.newScript("at_childrens_week_spot"); 
s->pAreaTrigger = &AreaTrigger_at_childrens_week_spot; 

you should instead use this (which is also possible with your patch):

s.newScript("at_childrens_week_spot")->pAreaTrigger = &AreaTrigger_at_childrens_week_spot; 

otherwise it looks nice

[evil-at-wow]

There is also no need to replace std::string with a const char* for the Script member. I assume you changed it because passing NULL (the default for the constructor's const char* argument) to the std::string constructor didn't work - and indeed, that's not something you should do! But there is no need for a change really: you can keep the std::string member of Script, and make the constructor something like this instead:
Script( const std::string& scriptName = std::string() ) : mName( scriptName ), ...

Perfectly backwards compatible too, even though I'd say it doesn't make much sense to create a script without a name. Eventually you'll need a name for the script anyway, as your examples show, so I'd personally even drop the default and require a name at construction. But that's just me, and I don't mind to keep a default for the sake of backwards compatibility :)

[boxa]

Thanks for the comments guys, i'm waiting a Schmoozerd reaction now.

[xfurry]

May I suggest a better naming? Something like:
ScriptHolder pNewScript
instead of
AutoScript s

[Lillecarl]

I think this is great boxa :)

On Wednesday, March 27, 2013, Radu wrote:

May I suggest a better naming? Something like:
ScriptHolder pNewScript
instead of
AutoScript s

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/51#issuecomment-15520302
.

[Schmoozerd]

@xfurry problem using pNewScript will make the lines even longer (and many registries are now already very long lines)

[VladimirMangos]

Real progress will be replace function structures by virtual methods overides for abstract Script class.
But this hard do in compatible way...

[xfurry]

@Schmoozerd
maybe ScriptHolder pScript This one should be shorter. Not as short as s but at least it looks better. :)

[Schmoozerd]

Prepared with these renamings:

• AutoScript -> ScriptRegistry
• s -> pScript

Some search'n'replace stuff: (RegEx for VC)
new Script;\n[ ]pNewScript->Name[ ]=[ ]"{[a-zA-Z0-9]_}";
new Script("\1");

Note: "instance_azjol-nerub" is consistently named in non-SD2 style

However i noticed that you didn't convert all the script registries, and also (as DasBlub mentioned already) some of the converted still are inefficiently converted (two lines instead of one)

Please provide (or let somebody provide) a full patch for all scripts :)

I did a few changes to make things easier, and pused them to my repo at https://github.com/Schmoozerd/scriptdev2/tree/rewrite_scriptRegistry

[boxa]

@Schmoozerd
thanks for your point of view, i now again in work, and i will looks more carefully in some days.

[xfurry]

What's the status of this?
As I said, there are only a few commits on my work desk before switching to milestone 0.8 and it would be nice to have this implemented by then.

@Schmoozerd what is exactly missing from this patch?

[Schmoozerd]

@xfurry - this is only the framework, and not yet applied to all files, hence not yet interesting in my view

[boxa]

oh, someone raised my old theme =)
now practically all old big script definations in my core reworked to use AutoScript class
and here even easier to determine ai (main purpose for this)

for example, instead of monstrous:

    Script* pNewScript;

    pNewScript = new Script;
    pNewScript->Name = "npc_depleted_war_golem";
    pNewScript->GetAI = &GetAI_npc_depleted_war_golem;
    pNewScript->pEffectAuraDummy = &EffectAuraDummy_npc_depleted_war_golem;
    pNewScript->RegisterSelf();

    pNewScript = new Script;
    pNewScript->Name = "npc_harrison_jones";
    pNewScript->GetAI = &GetAI_npc_harrison_jones;
    pNewScript->pQuestAcceptNPC = &QuestAccept_npc_harrison_jones;
    pNewScript->RegisterSelf();

    pNewScript = new Script;
    pNewScript->Name = "npc_emily";
    pNewScript->GetAI = &GetAI_npc_emily;
    pNewScript->pQuestAcceptNPC = &QuestAccept_npc_emily;
    pNewScript->RegisterSelf();

used simple:

    AutoScript s;
    s.newScript("npc_depleted_war_golem", &npc_depleted_war_golemAI::GetAI)->pEffectAuraDummy = &npc_depleted_war_golemAI::EffectAuraDummy;
    s.newScript("npc_harrison_jones", &npc_harrison_jonesAI::GetAI)->pQuestAcceptNPC = &npc_harrison_jonesAI::QuestAccept;
    s.newScript("npc_emily", &npc_emilyAI::GetAI)->pQuestAcceptNPC = &npc_emilyAI::QuestAccept;

only three line...

really, this is very good to use.
old example (outdated code, but even here you can see how it worked): mangosR2@0c3f790

maybe implement it in CMaNGOS, or extend it?

[Lillecarl]

Why are we going from std::string to const char*?

[Lillecarl]

Naming "AutoScript" is a bit misleading.
Going from c++ string to cstring is backwards?