Core/Movement: fix the issues regarding MovePoint not handling correc… by chaodhib · Pull Request #17028 · TrinityCore/TrinityCore

chaodhib · 2016-04-24T23:09:18Z

This fix issues regarding Motionmaster::MovePoint() not handling correctly rooted units:

Fix the bug that occurs when MovePoint is called on a rooted npc. The unit would previously "teleport" to the destination point.
Fix the bug that occurs when a unit moving by PointMovementGenerator is rooted. It does not continue its movement after it's unrooted.

The PR is not ready to get merged as is. This is a draft. It needs some polish but the fix seems to be working.

Testing done:

call MovePoint() on a static unit and see if that units moves to the point.
call MovePoint() on a moving unit and see if that units moves to the point
call MovePoint() on a rooted unit. The unit should resume its movement and move toward the point as soon as the root fades.
Root (by casting frost nova on it for example) a unit already moving by MovePoint(). The unit should resume its movement and move toward the point as soon as the root fades.

I tried to make the least amount of changes so that the PR is as readable as possible. I think however that the whole movement system needs some serious clean up and this means unreadable and massive PRs in the future I'm afraid.

…tly rooted units.

ccrs · 2016-04-24T23:14:44Z

ermmm, right xd

I've already done all the work here, lets not duplicate efforts xd
(I need to update my PR I know)

Keader · 2016-04-24T23:26:26Z

+void MotionMaster::ResumeMovement()
+{
+    MovementExpired();
+}


jackpoz · 2016-04-25T08:27:29Z

I think however that the whole movement system needs some serious clean up and this means unreadable and massive PRs in the future I'm afraid.

The idea is to have a PR with only cleanup changes, maybe little changes in small PRs (like "Renamed _something() to Something() ") to allow a reviewer spot possible issues

Aokromes · 2016-05-26T03:28:12Z

Any reason to don't merge this?

chaodhib · 2016-05-26T08:32:57Z

I need to clean it up a bit
I need to test the case where we root a controlled unit. Because of the way I implemented the solution (using MovementSlot::MOTION_SLOT_CONTROLLED,) there might be some issues.

ccrs · 2016-05-26T10:45:09Z

mutate is not the way to go.
Delay this till my PRs are merged (or work together with me to fasten things up) and lets make the solution together.

Treeston · 2016-07-16T15:59:39Z

@ccrs where is your pr on this?

Treeston · 2016-07-16T16:20:49Z

This PR introduces a critical bug with mob pathing.

Repro:

Freeze a target (ex. Frost Nova)
Stun the target (ex. Deep Freeze)
Have the freeze effect expire before the stun does
After the stun expires, the mob will not resume moving

ccrs · 2016-07-16T23:10:57Z

due to the nice logic of duplicating and separate work on 52343241 PRs its simply not updated, but got it working on everything

Treeston · 2016-07-16T23:15:01Z

@ccrs got a branch I can play with? Going through your open PRs and reviewing atm.

chaodhib · 2016-07-17T09:10:40Z

The PR has some holes like the scenario described by Treeston and is not ready to merged. However this could be solved by adding another layer in the MoveGen stack (remove the size limit of the stack which is now set to 3). I don't think the idea behind this PR should be dismissed since it's simpler than ccrs's solution because I'm reusing existing components.

Shauren · 2016-07-17T14:39:48Z

@chaodhib MoveGen should probably be rewritten to use std::priority_queue with a lot of possible slots
and possible priority (simplified, i know this isnt everything)
stun > root > script_controlled (MovePoint) > target chase > waypoints -> idle

Aokromes · 2017-06-03T06:09:10Z

Conflicting files
src/server/game/Movement/MovementGenerators/PointMovementGenerator.cpp
src/server/game/Movement/MovementGenerators/PointMovementGenerator.h

Keader · 2017-06-03T11:35:15Z

wow, part of my issues in razorscale is solved here o.o

chaodhib · 2017-06-03T12:17:17Z

I'm going to focus on #18771 first before I resume working on this PR. However, could you elaborate @Keader ?

Keader · 2017-06-03T12:21:57Z

@chaodhib in #19828 i'm having issues with razorscale making "teleport" instead move after add UNIT_STATE_ROOT and remove UNIT_STATE_ROOT.
And having issues too after remove UNIT_STATE_ROOT, he dont start chase movement again (i need force using me->GetMotionMaster()->MoveChase() )
If he wipes in last phase (ground phase, without unit_state_root) he start walk in the air instead make fly animation

2 of my issues looks like fixed by this pr

chaodhib · 2017-06-03T12:31:06Z

Ok, I'll investigate as soon as I can.

ccrs · 2017-06-04T21:32:28Z

@Keader totally not related, you simply erase slot_active with a movepoint....

to put it simple MovePoint erases MoveChase because they share the same slot. 1 : 1 script developer manual. no issue or bug on motionmaster.

ariel- · 2017-06-09T02:59:07Z

Agree that an api allowing to resume chase after movepoint would make scripting more intuitive.

Faq · 2017-11-12T11:03:18Z

@ccrs where to see your mentioned PR?

ghost · 2017-11-12T11:11:07Z

@Faq : his TrinityCore PRs are listed here: https://github.com/TrinityCore/TrinityCore/pulls/ccrs

Aokromes · 2018-03-11T00:27:41Z

Conflicting files
src/server/game/Movement/MovementGenerators/PointMovementGenerator.cpp
src/server/game/Movement/MovementGenerators/PointMovementGenerator.h

joschiwald · 2018-05-09T12:03:00Z

Superseded by #21888

Core/Movement: fix the issues regarding MovePoint not handling correc…

1d2152f

…tly rooted units.

Keader reviewed Apr 24, 2016
View reviewed changes

Aokromes added Feedback-PatchFix Comp-Core labels Apr 25, 2016

Aokromes added Feedback-FixOutdatedMissingWIP and removed Feedback-PatchFix labels Jun 5, 2016

Aokromes added the Branch-3.3.5a label Jul 15, 2016

Aokromes added the Feedback-WaitingResponse label Nov 27, 2016

Aokromes removed the Feedback-WaitingResponse label Dec 13, 2016

joschiwald closed this May 9, 2018

Aokromes removed Feedback-FixOutdatedMissingWIP labels Mar 31, 2019

Conversation

chaodhib commented Apr 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccrs commented Apr 24, 2016

Uh oh!

Keader Apr 24, 2016

Choose a reason for hiding this comment

Uh oh!

jackpoz commented Apr 25, 2016

Uh oh!

Aokromes commented May 26, 2016

Uh oh!

chaodhib commented May 26, 2016

Uh oh!

ccrs commented May 26, 2016

Uh oh!

Treeston commented Jul 16, 2016

Uh oh!

Treeston commented Jul 16, 2016

Uh oh!

ccrs commented Jul 16, 2016

Uh oh!

Treeston commented Jul 16, 2016

Uh oh!

chaodhib commented Jul 17, 2016

Uh oh!

Shauren commented Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aokromes commented Jun 3, 2017

Uh oh!

Keader commented Jun 3, 2017

Uh oh!

chaodhib commented Jun 3, 2017

Uh oh!

Keader commented Jun 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chaodhib commented Jun 3, 2017

Uh oh!

ccrs commented Jun 4, 2017

Uh oh!

ariel- commented Jun 9, 2017

Uh oh!

Faq commented Nov 12, 2017

Uh oh!

ghost commented Nov 12, 2017

Uh oh!

Aokromes commented Mar 11, 2018

Uh oh!

joschiwald commented May 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

chaodhib commented Apr 24, 2016 •

edited

Loading

Shauren commented Jul 17, 2016 •

edited

Loading

Keader commented Jun 3, 2017 •

edited

Loading