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

Implemented Book "Soothsaying for dummies" #16152

Merged
merged 1 commit into from
Jan 28, 2016

Conversation

ariel-
Copy link
Contributor

@ariel- ariel- commented Jan 3, 2016

Closes #4425
Loosely based on @elecyb work: elecyb/TrinityCore@fb0ca6a

Same as #13356, using DB for gossips (I had no idea back then D:)

@ghost
Copy link

ghost commented Jan 3, 2016

Good idea, it will be nice to have that profession feature working in-game. 👍

@ariel- ariel- force-pushed the soothsaying_for_dummies branch 2 times, most recently from e4ae737 to 639eda0 Compare January 4, 2016 00:20
@Keader
Copy link
Member

Keader commented Jan 4, 2016

@ariel- Fix works. But enters the same problem as all other NPCs "removing specialization"... after relog specialization back to spellbook

@ariel-
Copy link
Contributor Author

ariel- commented Jan 4, 2016

That is another bug, not in the scope of this PR to fix.

@elecyb
Copy link
Contributor

elecyb commented Jan 4, 2016

@Keader Here is the fix for removing specialization: https://gist.github.com/elecyb/fcc302bbcc8bb9bc8275
Code is a bit old, but may apply in the current code

@Keader
Copy link
Member

Keader commented Jan 4, 2016

@elecyb work, but does not break anything else?I could open up a PR about it?

@ariel- Forgiveness escape the main objective of the PR

@elecyb
Copy link
Contributor

elecyb commented Jan 5, 2016

@Keader yeah, feel free to PR it.
It doesn't break anything, the code removed is completely obsolete and pre-wotlk cmangos/mangos-svn@604224f
Excluded spells are all profession spells:

Effect_1=47 && Effect_2=0 AND NOT baseLevel

2656    Smelting
9787    Weaponsmith
9788    Armorsmith
10656   Dragonscale Leatherworking
10658   Elemental Leatherworking
10660   Tribal Leatherworking
17039   Master Swordsmith
17040   Master Hammersmith
17041   Master Axesmith
20219   Gnomish Engineer
20222   Goblin Engineer
26797   Spellfire Tailoring
26798   Mooncloth Tailoring
26801   Shadoweave Tailoring
28672   Transmutation Master
28675   Potion Master
28677   Elixir Master

Continue the discussion on the related issue #226

@ariel-
Copy link
Contributor Author

ariel- commented Jan 5, 2016

@Keader I mean I didn't look into that issue, but the book working should be independant of the core bug which relearns a tradeskill specialization.

@jackpoz
Copy link
Member

jackpoz commented Jan 7, 2016

does this work flawlessly now that the "fix learn on login" PR has been merged ? can this be merged ? hand on the ?

@Keader
Copy link
Member

Keader commented Jan 7, 2016

@jackpoz When tested, tested with two fixes applied (before the merge, had manually applied here) and was functioning normally.

@jackpoz
Copy link
Member

jackpoz commented Jan 7, 2016

is http://wotlk.openwow.com/npc=7944 supposed to offer http://wotlk.openwow.com/quest=3647 after I unlearn Gnomish Engineering with the book and show the "Train Me" option with Gnomish Engineering recipes ?

@jackpoz
Copy link
Member

jackpoz commented Jan 9, 2016

so ?

@elecyb
Copy link
Contributor

elecyb commented Jan 9, 2016

In pre-wotlk the renewal quests was required to get gnomish/goblin schematics from the trainer, after 3.1.0 the quest was removed and the schematics added to trainers. http://www.wowhead.com/item=11827/schematic-lil-smoky#comments:id=7687:reply=91409
For relearning spec, you just need to pay the fee and select the new one.
Here is a video that helped me with the initial implementation: https://www.youtube.com/watch?v=9ffNV2ZRI7M

Anyways, this implementation without the Leatherworking unlearn part, is incomplete.

@ariel-
Copy link
Contributor Author

ariel- commented Jan 9, 2016

So, the same book should be able to also change LW spec? Thought there was NPC for that.

@jackpoz
Copy link
Member

jackpoz commented Jan 9, 2016

Please let me know when this PR is complete once it's clear how it should be (LW/trainer behavior)

@elecyb
Copy link
Contributor

elecyb commented Jan 10, 2016

I mean learn LW spec in my previous comment, the unlearning is done by the npcs. To clarify, the book allows:

  • Unlearn engineering specialization
  • Learn engineering spec
  • Learn leatherworking spec

Here is the same implementation with the LW spec learning implemented: https://gist.github.com/elecyb/78fb41f78f6940466350

@ariel-
Copy link
Contributor Author

ariel- commented Jan 28, 2016

Makes me wonder, which one is the correct gossip whenever the player has both Engineering and Leatherworking?

@Treeston
Copy link
Member

Both, I'd assume.

@Treeston
Copy link
Member

Also, the LW trainers apparently offer to train you even if you don't have their spec. I think that's a bug?

@lonestarly
Copy link

I found I can learn Armorsmith with Weaponsmith Spec, Vice versa; I can learn Gnormish Engineer with Globin Engineer Spec, Vice versa. This did not happen with 2012ish TC patch, so it should be a bug.

@ghost
Copy link

ghost commented Apr 28, 2016

@lonestarly : if you find a bug, report it in a new issue, unless an issue already exists for that bug.
Make sure you search the issues (use the search function) for related bugs before opening a new issue.

@ariel- ariel- deleted the soothsaying_for_dummies branch May 6, 2016 01:10
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

7 participants