qsa

Pushed few arbitrary changes.

Feel free testing

This is interesting topic. Since I remember, overall dps on MaNGOS servers was much too high comparing to official servers.

Some tests from players found on wow community forums prove, that many (maybe all) % modifiers should be added, not multiplied by themselves.

This (probably) bug leads (example from my server) prot warriors to 1st place in arenas or battlegrounds (because this spec has A LOT of % modifiers and when they are multipied then they gain pretty big boost in damage).

The main reason for much bigger values are the stacking auras.

While this can also affect the final values, its effect is less that 1% overall.

Still nice thing to have tho.

Thanks.

Hi All,

I will try and build my server again. Just to ensure that we are on the same page, I'm using the code from faramir118's github

http://github.com/faramir118/mangos and qsa's patch

http://pastie.org/1248321 is this all I need?

Cheers

EDIT: No it's still not working for me, I must have missed something. All I did was

create standalone patch from faramir118's github, and cleaned the code

remove all 'trailing whitespaces' that git flagged

remove all 'leading whitespaces' that git flagged

remove all 'newline at EOF' that git flagged

fixed merge conflicts where core renamed functions canFly(), canSwim() and canWalk() to CanFly(), CanSwim() and CanWalk respectively

and applied this to the latest core.

I then applied qsa's patch and then I built it. This is what I get;

Pet                      Follow Automatically
Hunter pet                       yes
Warlock minion                   no
Companion                        no

It looks just about right.

But I have no idea what you are doing wrong, or what the problem really is.

I tried it once again, just to be triple sure, and it works for all types of pets.

Tested on both water elemental and succubus.

My tests are done on our repository + that patch.

Maybe you have problem with merging or applying the patch. Or maybe, its problem with other custom patches you use. It also can be addon or something like that.

The reason I'm so sure there's no problem, is that both hunter pets and lock pets use exactly the same movement code.

Best regards.

I've tested the mmap_rewrite with your latest patch and the minion still does not follow it's owner. I have a warlock with a succubus minion and it does not follow, but lags behind until beyond LOS, then it despawns.

Just tested, and it works like a charm for me.

So I cannot confirm this report.

qsa:

I like the changes in your most recent diff.

For pets, we will need to handle nopath in PetAI::UpdateAI (or PetAI::_needToStop).

If pet can't reach the target, PetAI::_stopAttack

If pet can't reach the owner, what is the expected behavior? Relocate to owner?

Right now, pets will try to come as close as they can to target/owner.

I think its more than reasonable. So, no changes in pet code needed.

Don't really remember how it is dealt with on retail, but its either that, or just taking no action at all.

Personally, I like the first option. It doesn't cost us nothing.

I'll give people time test it bit more, and push it.

Reluctance for Minions or Pets to follow the owner.

I thought you might be interested in this issue that occurs with the current 'mmap_rewrite' code.

Test: I created two identical server builds, on with the mmap_rewrite code and one without.

With mmap_rewrite code

Minions in particular will not automatically follow the owner. Selecting the 'follow' button does prompt the minion/pet to catch up with the owner, but then remain at that point until you press 'follow' again.

Without mmap_rewrite code

Minion/pets automatically follow owner unless commanded to 'stay'.

I have not found what is causing this, but suspect that something is interrupting 'MoveFollow'.

Hope this helps a little.

Thanks for testing.

This issue should be fixed in my latest test patch. Feel free retesting and report.

faramir118: Are you sure http://github.com/faramir118/mangos/commit/27980025db926e26fedf758cf1d4479f321a3386 is necessary?

those modes are far more useful than the debug. Simply since you can "play" in real time with parameters and see the effects.

Below attached test patch.

Implements evading and aggro changes on targets that become unreachable.

Pretty much everything we spoke about.

It uses the lazy logic. Which makes it simple yet effective.

The only thing missing (as far as I know) from all the things we spoke about, is evading timer. Right now its 0 delay. But since it is used only in very specific place, this feature can wait for later.

Feel free to test and report issues.

Patched over (c8a25250df7752) :: http://pastie.org/1248321

Ideas, remarks, criticism is always welcome.

Take care.

PS: Can anyone check that I remove the target from aggro list properly? lines 140 and 200 in diff . Thanks in advance.

http://pastebin.com/ijW0gGWD got this many times. Tried builtin ACE and not(5.7.8ver.) Core isn't clean but none of the patches impacts it. Crash appears randomly(got 3times at once during 10min.)

Core: 16040(not clean)

DB: YTDB_569

SD: ....

OS: Ubuntu 9.10

Can confirm this one.

Not very common, but happens from time to time.

Can post nice collection of dumps, but they are all same as above.

http://github.com/insider42/mangos/commit/e9ef7e36ae5929b3eb1f327673646f9302ff9fd0

also encounted a problem when i have DoStartMovement(pWho, 20.0f); or something like that in script - ofc mob will stay at 20 yards from you and use ranged spells, but it will EVADE after few secs because of evade code from mmap mod.

How can i fix this problem? Maybe there is some other way to prevent npc from trying to do melee for scripts? Or how i can check if mob is ranged before evade it (add some checks for mmap evade code)?

About that patch: we are not yet on mangos version that require this change. It will be included when we update.

About DoStartMovement : is is unforeseen side effect (when offset is used) from old evade code.

I am currently working on implementing better way.

This issue should be solved there.

I'll post it for testing, as soon as I have time for some basic checks.

Thanks.

PS: do you have issues with stability?

One more crash:

....

Why? Debug can resolve this crash? =)

This stack looks more logical, but, without line numbers and locals there's not much I can do.

Either that, or way to reproduce.

I don't see how it can crash, but sure, there always can be really strange corner cases.

So, ye, in a way, debug can solve the crash. Just as others already mentioned.

If anyone have *nix core dumps, don't be shy and post them here. If it is as common as KiriX say it is, you shouldn't have problem getting those.

Take care.

crashing every 5 min

08C78613  00000000  ScriptedInstance::DoUpdateWorldState+3
08BF331C  00000000  hyjalAI::Reset+8C
08BF2DB7  00000000  hyjalAI::hyjalAI+97
08BF2E9A  00000000  GetAI_npc_jaina_proudmoore+4A

SD2 related?

I can bet this is due to spawning Jaina outside hyjal, thus m_pInstance is empty, and it isn't always checked before use in scripts.

Very often crash at last version of mmaps =(

Call stack:
Address   Frame     Function      SourceFile
007A042A  00000000  PathInfo::findSmoothPath+9A
004806EA  00000000  MaNGOS::WorldObjectWorker<MaNGOS::NearUsedPosDo>::Visit+A
4589FFFE  00000000

This is not valid stack call.

Any chance you know how this can be reproduced?

Thanks in advance.

PS: anyone having any other related crashes? ( with *nix dumps preferably )

Should probably check the length of the charge's path, and return an error if it's too long. That would at least prevent the scenario in the video.

Its not real solution. Tho we can add it as temp hack. Not sure if we should deal with this at all.

But in general, it can be good idea to have the ability to limit the point path length dynamically.

...

A minor issue that I am getting when I compile the code from http://github.com/faramir118/mangos/tree/mmaps_rewrite/contrib/mmap

I'm getting this warning when I compile the current code with gcc on *nix

...

Hope this helps

Thanks for noticing this typo. I pushed the change into repo.

We welcome every little bit of help.

I noticed you added pathfinding to charge effects. When these fail, will the charge fail or will it do a shortcut? On retail servers, if you can't get a path to the target, you'll just get an error "No path to target found" or something similar.

It will "always" get you to destination (unless there's problem with mesh).

I never seen "path error" on retail, maybe its since LoS checks cover almost all such cases.

But from what I remember, you can charge across small gaps.

If we going to have the option to specify parameters for each and every specific map, maybe we can make bash script with all those "best" parameters for every map.

Some community input can be nice here. which values work the best for you for every map, etc.

PS: I pushed my recent changes into the repo.

I'll work on pushing your changes today.

I'll also make you a contributor...

It's like 5min of "work" splitting them and pushing (unless you do it manually, which I hope for you sake, you don't).

If you going to add me, I can push my changes on my own.

another "funny" bug: http://filekeeper.org/download/insider/different/loool_video.zip

heh, thanks for showing.

Ye, I aware of this kind of bugs, but unfortunately there isn't much I can do there right now.

The problem is with the mesh itself. I bet that step right there is blocking the path, so all that longer path is found instead.

You can check it by generating debug info while generating mmaps, then opening it with recastDemo and actually seeing all the walkable polygons.

The reason we cannot eliminate those things all together, is the same reason we still have sometimes bugs with vmaps ( LoS checks). There are just too many cases. We can't make it manually either All we can do is try to minimize those.

Maybe faramir118 or Gotisch can take a look at those. Since I haven't really spent time on the generator yet, thus don't fully understand it (yet).

In general this charge shouldn't be casted at all due to LoS fail. But, as I said in last post : LoS+terrain = bugs (at the moment atleast ).

I did my tests in BE arena, and it worked there pretty fine.

Take care.

qsa, do you have a github account? I can add you as a commiter, so this doesn't take so long.

I rather not, but if you really don't have time dealing with this, I'll add it. My nick there is sixsixnine .

works really good if objects that exist on caster's way _can_ break LOS.

But there is a problem when you stay under mountain and try to charge - because there is no path for this place caster will use (as i think) standard MonsterMove and fall through textures ofcouse (because of some falls by z coord every step when moving, yes you can increase "z" parameter when calling MonsterMoveByPath but this will cause a thing like jumps after charge into the air on this "increase value" if caster's and target's z is equal or target is below caster for z coord)

Firstly, thanks for testing.

About the issue you describe:

The real problem there is the value returned from GetContactPoint(). Target falls under map since point returned there ( in some cases ) below the actual terrain. This is due to VMAPs.

The way point path is generated for charge effects, is that it will always reach the final point given..

I do know this issue exists, but it is not directly related to mmap. Therefore, I think, all those changes and problems should be solved regardless of pathfinding usage.

For example, the last case scenario (and first) you gave has absolutely nothing to do with mmaps at all. It should be handled in LOS checks. But as we all know, at the moment terrain in not considered as obstacle in LOS calculation.

Just to sum it all up : someone should make independent patch solving those issues. I rather not do it as part of mmaps. Simply since it is not directly related and we don't want to make this project even bigger than it currently is.

Best regards.

Made few more changes.

* Everything from my previous patch ( post #445)

* Ability to specify which type of point path to generate. Either smooth or straight.

* Implement pathfinding in charge effects using straight path.

- Added Unit::MonsterMoveByPath() to simplify this case and allow usage in similar.

* added the ability to specify path type in ".debug mmap path" command. ".debug mmap path true" for straight path, smooth is default (or false).

patched over (213e909001b7) :: http://pastie.org/1232897

Take care.

...

- If you attack a mob while you're unreachable :

- he doesn't move from where he stands at the moment, he just faces us

- he attacks with ranged attacks/spells if he can, otherwise it does nothing.

- if he does not attack you with ranged attack/spell, he evades after precisly 26 seconds (don't ask me why).

It was the same 26 seconds for lvl 70, 71, 82 or 83 mob.

- if you become reachable before 26 seconds, he starts chasing you, and attacking you.

- he seems to be in a "nearly evading" state, because :

- as long as you're unreachable, you can't reach him either ("miss" all the time), unless he attacks you with ranged attacks/spells.

In that case, you cannot reach him all the time, but just when he casts his attack/spell (this part was a bit unclear for me...)

- he is constanly regenerating full life/mana (every 2 or 3 sec.).

- it seems like retail has some troubles too : I attacked a mob while I was unreachable several times.

Then, when I tried to attack him being reachable, he wasn't able to chase me anymore, he was rooted.

...

Thanks for testing.

This is not exactly how I remembered it, but close enough.

Are you sure that if you are unreachable target will not even try to come closer?

Overall this seems not very natural behavior to me. Don't really sure why. But its clear that "they" deal with "no full path"="something wrong" -> just error out ( evade ).

Can you please also test the scenario when you have 2 targets. Main target becomes unreachable, while the other still is. NPC will switch to 2nd target, but what happens with threat? something visible?

Thanks in advance.

Should we follow this type of behavior at all? I mean, do we really need to copy their bugs?

We can either try to mimic retail behavior using the fastest/most accurate solution, OR we can try to make something more reasonable.

But that's totally different question.

I think the part we really disagree on is the intent of pathfinding. Is it a tool to only build a point path, or should it be used for other things?

I think we disagree on the "how", not the purpose of pathfinding.

We can always make different class with only purpose will be checking reachablity. We sure can save some CPU cycles there, since we wont have to generate anything but poly paths + couple simple checks.

Then we can use this class for checking targets in SelectHostileTarget.

This is perfect solution. We'd know if we can reach target even before selecting it.

Unfortunately, this is where we really disagree. Perfect solution do not exist in our imperfect world.

Those checks are not free. If they were, we'd just use them.

Here is where the problems start. Making it happen.

If we just check all those targets and then generate path for chosen one we are wasting alot of resources.

If we try to somehow use the data we generated there to build the path, we are mixing AI with Movement.

Sure, you say : "lets make it so efficent, so it wont metter how many paths we generate, how many checks we run".

The best you can do here is cut the overhead as close as you can to A* cost, its our lower bound.

It is far, far from "free" or low.

Best regards

The advantage is that we have an algorithm that correctly emulates retail behavior, no matter the circumstances.

It's been a long time (couple years) since I played retail, but I remember how this part works.

In Magtheridon's Lair there is a spot on the door/portcullis where you could stand that monsters can't path to. If the tank jumped on there, Magtheridon would start killing other members of the raid. He would only reset if the entire raid (minus the dead people) were standing on that spot.

I'm almost 100% sure they do it by either dropping unreachable target or reducing its threat by %.

So we automatically have new target to attack, hoping we can get to him.

Maybe someone can test this on retail? There is no freaking way they generate paths to all targets at target selection.

We can do the same: no path -> SelectHostileTarget drops ~50% of the threat of main target -> next iteration.

Looks like we agree to disagree.

Therefore the only logical solution is to have 2 different versions. Test them properly. Only then (maybe) we will come to better conclusions, understanding of the good, the bad and the ugly about every way.

Gotisch : thanks for summarizing it all up. It is pretty accurate and will surely help people too lazy following the walls of text from above

EDIT: didn't want to make new post.

below patch adds support for extractor under linux.

CMakeLists.txt : http://pastie.org/1223272

code diff against 213e909001 :: http://pastie.org/1223273

cmake file can be used to generate any project under VC as well.

The source changes include mainly missing includes, warning fixes.

The only "real" issue was that *nix systems are case sensitive while win aren't.

Take care.

So, that is a wonderful modular logic, and this only has the disadvantage that the check if a target is reachable is (likely) the same work as creating a path to this target

That's the main problem. If we could check for reachibility "for free", there would not be any problem "just checking" before selecting target.

What you actually saying is same thing faramir118 does : all paths at once.

Sorry, I had to edit my previous post with some thoughts on the subject.

I'm looking for 100% guarantee that we pick a reachable target, assuming that there is one. I'm not convinced yet that what you propose (no pathfinding during target selection) is going to accomplish that. Even after several iterations, how can we guarantee it will settle on one reachable target? How can we guarantee correct behavior when there are 0 reachable targets?

iteration start (update call):

- MovementGenerator::

Generate path to current target.

- SelectHostileTarget::

Path generated for current target is valid -> noop.

else

mark current target in thread list as unreachable

select target with highest thread (and unmarked) from thread list (+ all current readability tests)

Iteration end.

If we have 0 valid (path wise valid) targets, out of X, it will take us X updates to figured that out.

By all means, it is finite, best effort. If here is valid reachable target, it will find it.

The MovementGenerator isn't aware of the big picture, and less-than-ideal circumstances may cause problems:

In the previous example, assume that Alice and Bob (and for the sake of argument, 8 other players) are on the roof. Eve's movement generator will reject each target in turn, but never all at the same time.

SelectHostileTarget is already dealing with the big picture. It already makes decisions based on location of the potential target, and pathfinding is a (really complex) extension of that functionality.

AI should use whatever information it has at its disposal to make the best choice possible. Categorically denying information based on cost is possible, but in this situation I think the cost is reasonable most of the time.

By the time SelectHostileTarget runs it has all the data it needs to make educated decision.

MovementGenerator don't have to know anything - it takes its commands from AI.

I know it's potentially costly to generate paths in SelectHostileTarget, but in most cases it should only generate (or update) one path. I think that it will take special circumstances (exploits, bugs in pathfinding, bad navmesh, etc) to create a true bottleneck. It would be better to catch and handle these root causes. For example: count the number of incomplete paths generated in SelectHostileTarget - if there are more than 5, log an error with information about what map, location, creature, etc.

Exactly the same applies to 2nd solution too. In most cases first target we select will also be the last.

In addition to the fact it is wont be used by anything but npc's which use targetmovement generator ( its maybe 1% of all units in any given moment).

Probably 99% of units which call SelectHostileTarget use TargetedMovementGenerator:

SelectHostileTarget calls AI()->AttackStart(). All core AI implementations of AttackStart() call MoveChase.

I'm not familiar enough with scriptdev2's AI implementations to know what to do to allow them to skip pathfinding. I'll post more on this after I look into it.

Sure, its just Players derive from Unit, Units not in combat do not chase anything. So it still stands : "maybe 1% of all units in any given moment".

Maybe I'm really failing to see the major advantages of generating paths as soon as SelectHostileTarget over generating them in movement generators.

Please do explain me.

I'll try to explain once again why I'm against generating paths in AI code.

Firstly and most importantly : it single handedly destroys entire modularity concept. By creating those ties between unrelated section of the code, unnecessary dependencies created.

Integrating them one into another just creates blob of code.

AI: what to do.

MovementGen: how to do.

We should not mix those.

2nd: path generated are valid until ANY target moves. If you generate X paths every update call, you are wasting alot of resources here for very, very little gain ( corner case ).

We may not want to solve it at all.

Consider it : as soon as primary target changes, the creature will switch target anyhow.

I think something like can be fair solution: if cannot reach main target, try one more target, if not evade.

The more I think about it, the faster I come to conclusion that we should not deal with this case at all.

All that has to be done, is to move the evading code out from movement generator to SelectHostileTarget and "call it a day".

If main target not reachable due to terrain - evade. This cannot be "natural" case.

Consider this : all thread list out of reach ( since they found hold in map and they are abusing it or something ).

How is that different from main tank constantly out of reach? I think it isn't.

Back to my point : We should just evade. End of story.

Thanks in advance.

For qsa's way:

update cycle 1:
   Creature::Update()
       Unit::Update()
           MotionMaster::UpdateMotion()
               MovementGenerator::Update()
(1)                 evadeWhenCan()
   (call stack eventually goes back to Creature::Update)
       unit is not evading yet, so UpdateAI()
           SelectHostileTarget
(*)             pick new target using pathfinding, set evadeWhenCan to false
(*)                 AttackStart()
(*)                     MotionMaster.MoveChase()

update cycle 2:
   Creature::Update()
       Unit::Update()
(2)         m_evadeWhenCan is true, so CreatureAI::EnterEvadeMode()
   nothing else matters, we are in evade mode (new MovementGenerator, leave combat, etc)

The stuff with (*) is not implemented yet.

Without it, everything between (1) and (2) is guaranteed to not matter (except if creature dies to aura damage).

I see no problem calling all those while we move in direction of something we consider "non-optimal" target. All the code in-between (1) and (2) should run anyway. If the creature dies, or whatever, we wont have to continue anyhow, already set evade flag wont change anything. Maybe the unit can cast spells while on its way, who knows ...

So it can be made to work, but only if you generate and validate paths in SelectHostileTarget - this is functionally the same as what I proposed.

No, By the time it gets to SelectHostileTarget, MovementGenerator already made path. There's no reason generating anything new here.

However:

From a design-oriented viewpoint, I feel that deciding to enter evade mode should solely be the responsibility of AI. Evade mode has a lot of implications besides just movement. MovementGenerator should only generate movement. It is a more logical separation of functionality, IMHO.

somehow copying them to movement master

This is easy, just store path in Unit instead of MovementGenerator.

That's right, "design-oriented viewpoint" - MovementGenerator deals with the movement, AI deals with the aggro.

That what I was saying all along.

That's why I'm against generating paths in SelectHostileTarget.

We can check the type of already generated path inthere, but not dealing with path logic.

Thats why we cannot store paths in Unit too. In addition to the fact it is wont be used by anything but npc's which use targetmovement generator ( its maybe 1% of all units in any given moment).

Keep in mind that if we will try to calculate paths to all feasible targets on same update we're creating bottleneck.

I don't know how often this will actually happen...

In most cases, players are solo and monsters will just reset (or attack the pet if there is one).

In raids, players are always right in the fight.

Another reason why we shouldn't add any heavy code dealing with this corner case.

EDIT: I took another look at the pseudocode you added for my suggestion, and it isn't exactly what I had in mind.

All 3 stars can be replaced with "select different feasible target" - no path generating involved there. Just checking what type the current path has.

And (1) can/should be removed ( after (*) added ).

PS: sorry for such late edit.

After thinking about it, I think it should stay the way it is in current mangos.

First, I will try to explain why I think, your idea wont work well ( atleast under current layout ).

We generate path under MovementGenerator. From what I understand, if we change the scheme to what you suggest, we will have to either generate paths as soon as isInAccessiblePlaceFor, and then somehow copying them to movement master, or generating those path more than once. This is totally unacceptable performance wise.

If we keep the current scheme, after we generate the path in (1), we get to (2) - there we can check the type of the path. If we can select better target, set new target for next MovementGenerator update.

We do "skip" one iteration here, but thats totally expendable when you compare it with generating extra paths ( checking reachability ).

Eventually we will select "the best target", it may take few extra update calls, but it will take overall less path calls.

Keep in mind that if we will try to calculate paths to all feasible targets on same update we're creating bottleneck. Spreading those calls is much better choice.

I think we should only modify Unit::SelectHostileTarget() to check current path type - if it isn't complete => try to select different target. No targets at all => evade ( after they came as close as possible etc .. ).

Just my thoughts, Ideas are welcomed. The more time we spend on planning, the less time we'll spend dealing with problems.

About the temp evade solution I have in my patch : for now its better than nothing. Can always be removed when better implemented.

It has nothing to do with actual pathfinding code, it just prevents loops when target is not reachable.

I still think it should be added for now.

PS: anyone had some time to do actual testings? don't be shy ...

[...]

extractor unix compatibility

You can run current under wine just fine, or just extract under win machine and transfer.