Core: Logical fixes in EventMap #7873

Merged
merged 1 commit into from Jan 30, 2013

Conversation

Projects
None yet
9 participants
@Gacko
Member

Gacko commented Sep 25, 2012

No description provided.

@Gacko

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Sep 25, 2012

Member
Member

Gacko commented Sep 25, 2012

@Gacko Gacko closed this Sep 25, 2012

@Gacko Gacko reopened this Sep 25, 2012

@Gacko

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Sep 25, 2012

Member

Missclick -.-

Member

Gacko commented Sep 25, 2012

Missclick -.-

@Gacko

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Sep 25, 2012

Member

This was the old issue, here's the fix as requested from @Subv

Member

Gacko commented Sep 25, 2012

This was the old issue, here's the fix as requested from @Subv

@Shauren

This comment has been minimized.

Show comment Hide comment
@Shauren

Shauren Sep 26, 2012

Member

Not convinced. All the scripts work without this

Member

Shauren commented Sep 26, 2012

Not convinced. All the scripts work without this

@Faq

This comment has been minimized.

Show comment Hide comment
@Faq

Faq Oct 4, 2012

Contributor

@Gacko So why this all needed if scripts working without it?

Contributor

Faq commented Oct 4, 2012

@Gacko So why this all needed if scripts working without it?

@Subv

This comment has been minimized.

Show comment Hide comment
@Subv

Subv Oct 5, 2012

Contributor

Yes, the scripts work without it, but they are limited to a maximum phase number of 7, when we could achieve a maximum of 8 phases, its a change that will allow creatures to have more eventphases in the future.

Contributor

Subv commented Oct 5, 2012

Yes, the scripts work without it, but they are limited to a maximum phase number of 7, when we could achieve a maximum of 8 phases, its a change that will allow creatures to have more eventphases in the future.

@Star-lion

This comment has been minimized.

Show comment Hide comment
@Star-lion

Star-lion Oct 5, 2012

Contributor

@Gacko let me know when you are totally finished with this and ill take a look at it.

Contributor

Star-lion commented Oct 5, 2012

@Gacko let me know when you are totally finished with this and ill take a look at it.

@Gacko

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Oct 7, 2012

Member

As Subv already told, we could use 8 phases. Further I don't think it's not the right usage of bitmasks even it's working... @Faq The way it's working isn't really pretty.

Member

Gacko commented Oct 7, 2012

As Subv already told, we could use 8 phases. Further I don't think it's not the right usage of bitmasks even it's working... @Faq The way it's working isn't really pretty.

@Gacko

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Oct 8, 2012

Member

It's mostly done so far but there are some questions left I'd like to talk about. @kandera I tried to contact you, but you weren't there...

Member

Gacko commented Oct 8, 2012

It's mostly done so far but there are some questions left I'd like to talk about. @kandera I tried to contact you, but you weren't there...

@Gacko

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Oct 16, 2012

Member

I implemented the IsInPhase() method now. We're settin phases with absolute numbers and check them with masks where most checks are only for one single phase. This combination doesn't make sense in my opinion.

EDIT: My method is way easier to handle than checking the mask on your own everytime. If someone wants to check multiple phases he can still do this on the "old" way. But then a method like SetPhaseMask would be nice for the sake of completeness.

I kept the _MASK constants in enums for backup. @kandera you can check it now, my work is done - for now.

Member

Gacko commented Oct 16, 2012

I implemented the IsInPhase() method now. We're settin phases with absolute numbers and check them with masks where most checks are only for one single phase. This combination doesn't make sense in my opinion.

EDIT: My method is way easier to handle than checking the mask on your own everytime. If someone wants to check multiple phases he can still do this on the "old" way. But then a method like SetPhaseMask would be nice for the sake of completeness.

I kept the _MASK constants in enums for backup. @kandera you can check it now, my work is done - for now.

@Gacko

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Oct 16, 2012

Member

Further: Correct me if I'm wrong, but there's no possibility to set multiple phases. It makes sense to handle masks but then there should be a possibility to set multiple phases :D

Member

Gacko commented Oct 16, 2012

Further: Correct me if I'm wrong, but there's no possibility to set multiple phases. It makes sense to handle masks but then there should be a possibility to set multiple phases :D

@DDuarte

View changes

.../scripts/Northrend/UtgardeKeep/UtgardeKeep/boss_ingvar_the_plunderer.cpp
- PHASE_EVENT
+ PHASE_EVENT,
+
+ PHASE_EVENT_MASK = 1 << PHASE_EVENT - 1

This comment has been minimized.

Show comment Hide comment
@DDuarte

DDuarte Oct 16, 2012

Member

Not being used?

@DDuarte

DDuarte Oct 16, 2012

Member

Not being used?

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Oct 16, 2012

Member

#7873 (comment)

Last line: "I kept the _MASK constants in enums for backup.". I can remove them if you want me to. :)

@Gacko

Gacko Oct 16, 2012

Member

#7873 (comment)

Last line: "I kept the _MASK constants in enums for backup.". I can remove them if you want me to. :)

@Gacko Gacko closed this Nov 13, 2012

@Gacko Gacko reopened this Nov 13, 2012

@ghost ghost assigned Subv Nov 25, 2012

@Shauren

View changes

src/server/game/AI/CreatureAIImpl.h
+ // Returns wether the eventmap is in the given phase or not.
+ bool IsInPhase(uint32 phase)
+ {
+ return phase <= 8 && (!phase || _phase & (1 << phase - 1));

This comment has been minimized.

Show comment Hide comment
@Shauren

Shauren Jan 22, 2013

Member

Derp. This won't work
return phase <= 8 && (!phase || _phase & (1 << phase + 23));
will - look at how _phase is constructed in SetPhase method

@Shauren

Shauren Jan 22, 2013

Member

Derp. This won't work
return phase <= 8 && (!phase || _phase & (1 << phase + 23));
will - look at how _phase is constructed in SetPhase method

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Jan 22, 2013

Member

Oh, yes, you're right. I'll think about it, thx :)

@Gacko

Gacko Jan 22, 2013

Member

Oh, yes, you're right. I'll think about it, thx :)

@ghost ghost assigned Gacko Jan 23, 2013

Gacko added a commit that referenced this pull request Jan 30, 2013

Merge pull request #7873 from Gacko/eventphases
Core: Logical fixes in EventMap

@Gacko Gacko merged commit 2ba477c into TrinityCore:master Jan 30, 2013

@Gacko

This comment has been minimized.

Show comment Hide comment
@Gacko

Gacko Jan 30, 2013

Member

This comment has been minimized.

Show comment Hide comment
@Norfik

Norfik Jan 30, 2013

Contributor

Gacko: "See http://www.trinitycore.org/f/topic/7821-eventmap-system for details."

The page you are looking for cannot be found.

Thx Aokromes xD Me bad hahaha....

Contributor

Norfik replied Jan 30, 2013

Gacko: "See http://www.trinitycore.org/f/topic/7821-eventmap-system for details."

The page you are looking for cannot be found.

Thx Aokromes xD Me bad hahaha....

This comment has been minimized.

Show comment Hide comment
@Aokromes

Aokromes Jan 30, 2013

Owner

You need to be logged in forum to see it.

Owner

Aokromes replied Jan 30, 2013

You need to be logged in forum to see it.

@Destalker

This comment has been minimized.

Show comment Hide comment
@Destalker

Destalker Jan 30, 2013

This looks strange....

raczman pushed a commit to raczman/TrinityCore that referenced this pull request Apr 20, 2014

Merge pull request #7873 from Gacko/eventphases
Core: Logical fixes in EventMap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment