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

[3.3.5] Bosses respawning #18824

Closed
Keader opened this issue Jan 13, 2017 · 38 comments
Closed

[3.3.5] Bosses respawning #18824

Keader opened this issue Jan 13, 2017 · 38 comments

Comments

@Keader
Copy link
Member

Keader commented Jan 13, 2017

Description: Bosses can respawn after you kill him.

Expected behaviour: Boss should never respawn after you kill him (after state = DONE).

Steps to reproduce the problem:

  1. Kill some boss
  2. Wait respawn

It was fixed by this commit: f826ecb
But reverted in : b3d44d6

Now need a new solution to fix Bosses without entry in linked_respawn.

Branch(es): 3.3.5

TC rev. hash/commit: a17e367

@joschiwald
Copy link
Contributor

how is it even possible that a boss can respawn?

@Treeston
Copy link
Member

Treeston commented Jan 13, 2017

Instance extension works now, they have a 1 week respawn timer (most of them) in the DB.

Core doesn't have any option for "never respawn".

@joschiwald
Copy link
Contributor

maybe that will be a better solution to implement a never respawn option like -1 in creature.spawntimesecs

@sirikfoll
Copy link
Contributor

The -1 in creature.spawntimesecs may not be effective because of the _DespawnAtEvade funcionality, it calls a respawn that should be allowed in that case, checking bossState is the simpler way I can think of.

@joschiwald
Copy link
Contributor

that are 2 different respawn triggers, db respawntimer triggers on death, _DespawnAtEvade is a manuell respawn trigger

@Treeston
Copy link
Member

What @joschiwald says is theoretically correct, but would require us to rewrite respawn logic.

I don't know if the instance boss case is widespread enough for that.

@joschiwald
Copy link
Contributor

or maybe spawntime 0 because gameobject uses negativ spawntime to determine despawned by default (see loot chests)

@Treeston
Copy link
Member

Treeston commented Jan 14, 2017

spawntime 0 is instant respawn iirc

ps: keep in mind the fix in #18827 is just a temporary fix, it'll be unnecessary once we have dynspawn (spawn groups for each boss + its trash toggled on boss state update by instance script).

@Keader
Copy link
Member Author

Keader commented Jan 14, 2017

So why remove the old fix if dynspawn is not ready? This modification just broken things and i cant see reason...
That modification should be done, when dynspawn is ready.

ps: keep in mind the fix in #18827 is just a temporary fix

Much work for a temp fix, no?
Why not restore the old behavior until dynamic spawn be ready?

@Treeston
Copy link
Member

Treeston commented Jan 14, 2017

Because adding more and more differences that need to be fixed before dynspawn is ready will not lead to dynspawn ever being ready.

PS: "much work"? what? This is a matter of replacing two elements per boss (inherit BossCreatureScript instead of CreatureScript, and change the ctor to pass the boss ID).

@Keader
Copy link
Member Author

Keader commented Jan 14, 2017

yes, we need change it in every single script that dont have entrys in linked_spawn, and use it for every single new scripts... instead be inside of BossAI (that dont need change nothing).
I realy cant see any benefit in b3d44d6, because now need change more stuffs, so to me you still be "adding more and more differences that need to be fixed before dynspawn is ready "

@Treeston
Copy link
Member

The existing system (CanSpawn in AI) is simply not compatible with dynspawn. It's that simple.

The system needs to be removed for dynspawn to work.

@djbarrueco
Copy link

djbarrueco commented Jan 17, 2017

Confirmed. We defeated Lord Marrowgar, Lady Deathwhisper, Gunship, Deathbringer Saurfang, Festergut and Rotface in the early hours of 15/01/2017. Monday morning Lady Deathwhisper was alive, not the rest of the mentioned bosses. The same situation is present at the moment (17/01/2017).

@Keader
Copy link
Member Author

Keader commented Jan 17, 2017

Only Lady Deathwhisper and Blood Prince Council should respawn in ICC (caused by this bug), because other bosses has entry in linked_respawn.
I disagree with fix proposed in #18827, is much work to a "temp fix".
Maybe implement a "DB flag" like Joschiwald suggest...
Btw, i cant do anything before finish illidan.

@Treeston
Copy link
Member

@Keader I wonder...should all creatures with bind flag never respawn naturally (unless spawn time override)?

@Keader
Copy link
Member Author

Keader commented Jan 17, 2017

@Treeston I think so

@Treeston
Copy link
Member

hrm, don't we have dungeon boss flag in there somewhere

if so we could just not schedule respawn if the respawn timer isn't specifically provided (in death logic)

@ariel-
Copy link
Contributor

ariel- commented Jan 19, 2017

The existing system (CanSpawn in AI) is simply not compatible with dynspawn. It's that simple.

Why not? Can't it be made to call AI to check spawn possibility?

@Treeston
Copy link
Member

There is no AI object before the creature is spawned, because there is no creature object before it's spawned.

@djbarrueco
Copy link

ps: keep in mind the fix in #18827 is just a temporary fix

#18827 does not work, Lady Deathwhisper reappears after 24 hours.

@Keader
Copy link
Member Author

Keader commented Jan 21, 2017

@noryad you need make some changes in lady script to make that pr works.
Try add it:

diff --git a/src/server/scripts/Northrend/IcecrownCitadel/boss_lady_deathwhisper.cpp b/src/server/scripts/Northrend/IcecrownCitadel/boss_lady_deathwhisper.cpp
index f178498..57abd43 100644
--- a/src/server/scripts/Northrend/IcecrownCitadel/boss_lady_deathwhisper.cpp
+++ b/src/server/scripts/Northrend/IcecrownCitadel/boss_lady_deathwhisper.cpp
@@ -185,10 +185,10 @@ class DaranavanMoveEvent : public BasicEvent
         Creature& _darnavan;
 };
 
-class boss_lady_deathwhisper : public CreatureScript
+class boss_lady_deathwhisper : public BossCreatureScript
 {
     public:
-        boss_lady_deathwhisper() : CreatureScript("boss_lady_deathwhisper") { }
+        boss_lady_deathwhisper() : BossCreatureScript("boss_lady_deathwhisper", DATA_LADY_DEATHWHISPER) { }
 
         struct boss_lady_deathwhisperAI : public BossAI
         {

But again, this solution dont looks good to me :/

@Treeston
Copy link
Member

i think easiest proper fix is a test when scheduling respawn (same method called by despawnonevade). if dungeon boss flag + no override respawn time -> never respawn.

will look into it.

@ariel-
Copy link
Contributor

ariel- commented Jan 21, 2017

There is no AI object before the creature is spawned, because there is no creature object before it's spawned.

Strange then, how did the prev system work?

@Treeston
Copy link
Member

One creature object per GUID, recycled on respawn. Which is a horrible hack.

@djbarrueco
Copy link

djbarrueco commented Jan 22, 2017

@Keader now it works ok

@djbarrueco
Copy link

djbarrueco commented Jan 25, 2017

How would be for boss_blood_council? The following does not work for me...

class boss_blood_council_controller : public BossCreatureScript
{
    public:        
	boss_blood_council_controller() : BossCreatureScript("boss_blood_council_controller", DATA_BLOOD_PRINCE_COUNCIL) { }

@Treeston
Copy link
Member

IDK if princes have linked respawn with their controller.

@Keader
Copy link
Member Author

Keader commented Jan 25, 2017

I removed princes spawn (perma spawn), they are summoned by controller now (like illidari council in black temple).
So, if they respawn is because controller respawn. Controller has BossAI and using that code should never respawn.

diff --git a/src/server/scripts/Northrend/IcecrownCitadel/boss_blood_prince_council.cpp b/src/server/scripts/Northrend/IcecrownCitadel/boss_blood_prince_council.cpp
index a6ec97c..fbab3f9 100644
--- a/src/server/scripts/Northrend/IcecrownCitadel/boss_blood_prince_council.cpp
+++ b/src/server/scripts/Northrend/IcecrownCitadel/boss_blood_prince_council.cpp
@@ -208,10 +208,10 @@ uint32 const PrincesData[] =
     DATA_PRINCE_VALANAR
 };
 
-class boss_blood_council_controller : public CreatureScript
+class boss_blood_council_controller : public BossCreatureScript
 {
     public:
-        boss_blood_council_controller() : CreatureScript("boss_blood_council_controller") { }
+        boss_blood_council_controller() : BossCreatureScript("boss_blood_council_controller", DATA_BLOOD_PRINCE_COUNCIL) { }
 
         struct boss_blood_council_controllerAI : public BossAI
         {

@noryad BossState = 3 (DONE) when you build this code ?

btw @Treeston any news about #18824 (comment) ? xd

@Keader
Copy link
Member Author

Keader commented Jan 27, 2017

@Treeston Why not link this "NO-RESPAWN" to flags like: CREATURE_FLAG_EXTRA_DUNGEON_BOSS or CREATURE_FLAG_EXTRA_INSTANCE_BIND ?
All bosses should has this flags

@djbarrueco
Copy link

djbarrueco commented Jan 27, 2017

@Keader when i tested it, Lady Deathwhisper and Blood Prince Council had been defeated and reappeared after 24 hours. with the temporary fix Lady Deathwhisper does not appear in the room, the opposite of Blood Prince Council. Maybe if we defeat the Blood Prince Councile with the patch from the beginning it works ok.

@Keader
Copy link
Member Author

Keader commented Jan 28, 2017

My suggestion about this issue is:

https://gist.github.com/Keader/70286275ce21fc6f4530f42f121b4bf7

@Treeston
Copy link
Member

That breaks DespawnAtEvade

@Treeston
Copy link
Member

Treeston commented Feb 7, 2017

Re-tagging #18827 for fix

@djbarrueco
Copy link

#18827 merged in 94a6dd7
At this time it works ok for 10 man, not for 25 man, current revision 76f3e0e

lady_1

@Treeston
Copy link
Member

What is the output of .npc info

@djbarrueco
Copy link

lady info

@Treeston
Copy link
Member

Treeston commented Feb 19, 2017

Interesting, she doesn't get instance boss flag. No entry in instance_encounters? Entry is there. Investigating.

Treeston added a commit that referenced this issue Feb 19, 2017
…DUNGEON_BOSS) now properly propagates to all difficulty entries (not just difficulty 0).

This lets us simplify IsDungeonBoss() on Creature (and move it to header).

Closes #18824.
@Keader
Copy link
Member Author

Keader commented Feb 19, 2017

Thank you @noryad

Shauren pushed a commit that referenced this issue Jul 21, 2019
…DUNGEON_BOSS) now properly propagates to all difficulty entries (not just difficulty 0).

This lets us simplify IsDungeonBoss() on Creature (and move it to header).

Closes #18824.

(cherrypicked from 1beb2e5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants