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/Battleground: Removed _CheckSafePositions, this is handled by areatriggers now. #14570

Closed
wants to merge 0 commits into from

Conversation

Golrag
Copy link
Contributor

@Golrag Golrag commented Apr 18, 2015

Removed _CheckSafePositions, this is handled by areatriggers now.

Any feedback is welcome ! :)

About the areatriggers for starting areas, should I move them to Battleground::HandleAreatrigger or handle them in the specific battlegroundclass ? (Like BattlegroundTP::HandleAreatrigger)

Edit: Updated to 007ab5b

Added missing alterac valley areatriggers
Removed m_StartMaxDist

@Takenbacon
Copy link
Contributor

👍

@Golrag Golrag force-pushed the MiscBattlegroundsAreaTriggers branch from fe250f3 to 27e0f75 Compare April 20, 2015 14:28
@jackpoz
Copy link
Member

jackpoz commented Apr 21, 2015

I see https://github.com/TrinityCore/TrinityCore/pull/14570/files#diff-2d7bc8b5f9669823a0062232320c5d19L313 has been removed, where is it handled in the new code ?

@Golrag
Copy link
Contributor Author

Golrag commented Apr 21, 2015

It's not handled in the new code, I can do it if you want, but I didn't feel like it should, cause on retail it is easy to leave those areatriggers (well, it's easy in Twin Peaks horde or the other one with pandaren mine, goblin mine...). So if you want me to use it, I would like to remove the "(possible expoit)" part :p

@Golrag Golrag force-pushed the MiscBattlegroundsAreaTriggers branch 3 times, most recently from ef14a12 to 3203e3e Compare May 4, 2015 16:17
@Carbenium
Copy link
Member

@Golrag: is this PR finished?

@Golrag
Copy link
Contributor Author

Golrag commented Jul 10, 2015

@Carbenium haven't looked at this PR for a long time. Will take a look at it asap
Edit: this is finished :)

@ghost
Copy link

ghost commented Jul 10, 2015

The title should contain a short description after "Core/Battleground:", something like
Core/Battleground: Remove m_StartMaxDist and _CheckSafePositions
Maybe this is just cosmetic, but it should be fixed for the commit log
(unless the content of the PR is authored directly into the source with a closing reference back to this PR).

@Carbenium Carbenium changed the title Core/Battleground: Core/Battleground: Removed _CheckSafePositions, this is handled by areatriggers now. Jul 10, 2015
@Takenbacon
Copy link
Contributor

@Golrag Can you bring back the old _CheckSafePositions() checks, but keep the area triggers you've added? While area triggers are more accurate and cleanly handled, there needs to be some sort of fallback because they are also very easily exploitable (e.g. all one would have to do is prevent the client from sending the CMSG_AREATRIGGER opcode and they would never get teleported back, which is easier than it sounds).

@Golrag
Copy link
Contributor Author

Golrag commented Aug 9, 2015

@Takenbacon mh, the purpose of this PR is to replace the current implementation with a more blizzlike one. But tbh, I don't know what happens on retail when you get out of the starting zone without triggering CMSG_AREATRIGGER

@Aokromes
Copy link
Member

Aokromes commented Nov 8, 2015

This branch has conflicts that must be resolved
Use the command line to resolve conflicts before continuing.

@Golrag
Copy link
Contributor Author

Golrag commented Nov 8, 2015

Woops, will have to re-create everything

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.

None yet

5 participants