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 new commands, option to disable talent templating and refactor code. #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arenacraftwow
Copy link

@arenacraftwow arenacraftwow commented Mar 26, 2020

Like the commit message says. I've backported the features I had on my skirmish arena server. The commands I've added were working as intended there and were used heavily. But I've also cleaned up my code so some issues might have slipped in (altho I've tried to continuously test out all the differences I've made to my original version).

I'm gonna use this version on my server so if I find anything I will let u know.

Stuff added/changed

  • [NEW] copy command - a GM can clone a player, he can then use that player to create a new template
  • [NEW] save command - a GM can update a template based on his current gear.
  • [FIX] Fixed a crash if someone used the (hidden) gossip option 32
  • [FIX] Shouldn't generate as many Player (xx) tries to learn already active spell warnings
  • [CODE] A lot of code cleanups and manual deduplications, also run my clag formatter thru the code so some changes are caused by it. Might include have slight performance improvements.
  • Slightly mitigated Talent spells are taught to the player, not granted as talents #11 by adding a compile time constant to remove this feature. On my server I just don't use talent templating, just running with the mod-learn-highest-talent. By default talents will be left in tact as they are currently but this gives the ability to remove that feature in a build if needed.

@BarbzYHOOL
Copy link
Member

can you add the corresponding commands in sql command table in a sql file?

I also suggest to change ".templatenpc" into the simpler ".template" as there is nothing NPC related in that command

@arenacraftwow
Copy link
Author

@BarbzYHOOL Will do.

@arenacraftwow
Copy link
Author

@BarbzYHOOL Done

@BarbzYHOOL
Copy link
Member

"Slightly mitigated #11 by adding a compile time constant to remove this feature. On my server I just don't use talent templating, just running with the mod-learn-highest-talent. By default talents will be left in tact as they are currently but this gives the ability to remove that feature in a build if needed."

I don't understand. What is the solution actually? is there a solution?

@BarbzYHOOL
Copy link
Member

well after my suggestions, i guess we can merge, i quickly checked the code but i'm not the most qualified for that but nobody is working on the module at the moment anyway

Copy link
Author

@arenacraftwow arenacraftwow left a comment

Choose a reason for hiding this comment

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

"Slightly mitigated #11 by adding a compile time constant to remove this feature. On my server I just don't use talent templating, just running with the mod-learn-highest-talent. By default talents will be left in tact as they are currently but this gives the ability to remove that feature in a build if needed."

I don't understand. What is the solution actually? is there a solution?

It means that you can easly disable talent/glyph templating. So that the NPC only does Gear related stuff, which is enough for my use case.

@arenacraftwow arenacraftwow force-pushed the master branch 2 times, most recently from ef66290 to 3f067a0 Compare March 29, 2020 11:46
@BarbzYHOOL
Copy link
Member

well looks good, you tried the new command names tho? just in case a typo or smthg

@arenacraftwow
Copy link
Author

seems to still work.

@@ -6,16 +6,28 @@ Set @NpcName = "Pick a spec",
@NpcDisplayID = 31833,
@NpcLevel = 80;

INSERT INTO `creature_template` (`entry`, `modelid1`, `name`, `subname`, `minlevel`, `maxlevel`, `faction`, `npcflag`, `speed_walk`, `speed_run`, `scale`, `rank`, `dmgschool`, `BaseAttackTime`, `RangeAttackTime`, `unit_class`, `unit_flags`, `unit_flags2`, `dynamicflags`, `family`, `trainer_type`, `trainer_spell`, `trainer_class`, `trainer_race`, `type`, `type_flags`, `lootid`, `pickpocketloot`, `skinloot`, `resistance1`, `resistance2`, `resistance3`, `resistance4`, `resistance5`, `resistance6`, `spell1`, `spell2`, `spell3`, `spell4`, `spell5`, `spell6`, `spell7`, `spell8`, `PetSpellDataId`, `VehicleId`, `mingold`, `maxgold`, `AIName`, `MovementType`, `HoverHeight`, `HealthModifier`, `ManaModifier`, `ArmorModifier`, `RacialLeader`, `movementId`, `RegenHealth`, `mechanic_immune_mask`, `flags_extra`, `ScriptName`, `VerifiedBuild`) VALUES
INSERT INTO `creature_template` (`entry`, `modelid1`, `name`, `subname`, `minlevel`, `maxlevel`, `faction`, `npcflag`, `speed_walk`, `speed_run`, `scale`, `rank`, `dmgschool`, `BaseAttackTime`, `RangeAttackTime`, `unit_class`, `unit_flags`, `unit_flags2`, `dynamicflags`, `family`, `trainer_type`, `trainer_spell`, `trainer_class`, `trainer_race`, `type`, `type_flags`, `lootid`, `pickpocketloot`, `skinloot`, `resistance1`, `resistance2`, `resistance3`, `resistance4`, `resistance5`, `resistance6`, `spell1`, `spell2`, `spell3`, `spell4`, `spell5`, `spell6`, `spell7`, `spell8`, `PetSpellDataId`, `VehicleId`, `mingold`, `maxgold`, `AIName`, `MovementType`, `HoverHeight`, `HealthModifier`, `ManaModifier`, `ArmorModifier`, `RacialLeader`, `movementId`, `RegenHealth`, `mechanic_immune_mask`, `flags_extra`, `ScriptName`, `VerifiedBuild`) VALUES
Copy link
Member

Choose a reason for hiding this comment

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

adding a DELETE before all this INSERT should be better I think

Copy link
Member

Choose a reason for hiding this comment

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

wait I just realize all these are for the WORLD db not the characters DB

omfg i should have merged my PR before

@arenacraftwow do you mind if I merge this old PR before this one? https://github.com/azerothcore/mod-npc-talent-template/pull/16/files it might create small conflicts though

Copy link
Author

Choose a reason for hiding this comment

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

@BarbzYHOOL I dunno, can't you just merge this and then adjust your PR. Then you could do all the DB changes you would like.

@P-Kito
Copy link

P-Kito commented Mar 30, 2020

Attach the * sign to the type instead of the name

@arenacraftwow
Copy link
Author

@P-Kito Well looks like I forgot to reformat the header file.

@arenacraftwow
Copy link
Author

Please just merge it or leave some serious criticism of the code. It's tiresome having to wake up and re-adjust these minor things, everyday since the past days.

@chamorrorenzo
Copy link

Hello there, Im trying this atm, and is amazing, I too wanted to stop using this cause of the talents learned beeing bugged when you use any option then its imposible to remove them from a player

im guessing this is where you change the option
static constexpr bool OMIT_TALENT_AND_GLYPH_TEMPLATING = false;

another question, when I save a template, it adds the new to the DB but the previus one still exist, is there any conflict with that? or its better delete the previus one

thanks

@mindsear
Copy link
Collaborator

Sounds complicated

@pangolp
Copy link

pangolp commented Sep 6, 2023

Can this pull request be closed? From what I see, a lot of time has passed, almost 3 years, since its creation and I don't know if they are going to really take it back.

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.

7 participants