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

Core/Phases: Fix SetInPhase not honoring zone phase #20156

Closed
wants to merge 4 commits into from
Closed

Core/Phases: Fix SetInPhase not honoring zone phase #20156

wants to merge 4 commits into from

Conversation

roc13x
Copy link
Contributor

@roc13x roc13x commented Aug 19, 2017

Changes proposed:

  • Fix SetInPhase not honoring zone phases, only area
  • Update area and zone phases on quest update

Target branch(es):

  • master

Tests performed:

  • Compiled
  • Tested in-game

Also update area phases on quest update
if (sConditionMgr->IsObjectMeetToConditions(this, phase.Conditions))
return false;
PhaseInfo const& phases = sObjectMgr->GetAreaAndZonePhases();
for (PhaseInfo::const_iterator itr = phases.begin(); itr != phases.end(); ++itr)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this - couldn't you make another store in ObjectMgr to lookup phase by id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that require quite a bit of nearly-duplicate code to construct a similar store? Especially in ConditionMgr::addToPhases

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean something like map<phaseId, vector<PhaseInfoStruct const*>> in ObjectMgr (constructed in same place as _phases)

@@ -16005,7 +16005,7 @@ void Player::SendQuestUpdate(uint32 questId)
}

UpdateForQuestWorldObjects();
SendUpdatePhasing();
UpdateAreaAndZonePhase();
Copy link
Member

Choose a reason for hiding this comment

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

This was previously a target of discussion in another PR - by forcing the update here you break quests that should not update phase here

Map by phaseid instead of area. No function actually needed to lookup phases by areaid.
Also reverted change in SendQuestUpdate
@Aokromes
Copy link
Member

@Shauren this can be merged?

@Faq
Copy link
Contributor

Faq commented Nov 24, 2017

/Me summons @Shauren and @joschiwald

@msoky
Copy link
Contributor

msoky commented Jan 10, 2018

any news?

@ghost
Copy link

ghost commented Jan 10, 2018

Good question, but I don't see any recent development here.

BTW, is this PR affected by Core/Object: fix logic in WorldObject::UpdateAreaAndZonePhase() #21214 and its commit 5b90538 ?

@Aokromes
Copy link
Member

Conflicting files
src/server/game/Entities/Object/Object.cpp

@ghost
Copy link

ghost commented Jan 10, 2018

Hardly any activity from @roc13x since October 2017 (view the user page).

@Shauren
Copy link
Member

Shauren commented Jan 10, 2018

I started working on a bunch of phase related optimizations that will make this PR obsolete (you can see it here, accidentally pushed new files for this bb718b5#diff-26a3ae191cac89dc294ec6b735af10ba)

@Shauren Shauren closed this Jan 10, 2018
@roc13x
Copy link
Contributor Author

roc13x commented Jan 10, 2018

Ok good to know. Saves me reviewing this tonight since 5b90538 happened

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants