Page MenuHomeWildfire Games

Combine Goal computation logic from MoveToPoint and MoveToTarget
ClosedPublic

Authored by wraitii on Jun 15 2019, 9:46 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22450: Unit Motion - combine Goal computation logic from MoveToPoint and MoveToTarget
Summary

MoveToPoint and MoveToTarget both compute a goal from a move request. They can be combined.

This allows using ComputeGoal everywhere a goal is needed, which lets us remove m_FinalGoal downstream.

NB: this commit is more dangerous than others since mistakes here could lead to stuck units easily.

Test Plan

Review the merge, compile, test in game.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/1736/display/redirect

Stan added a subscriber: Stan.Jun 15 2019, 11:52 PM
Stan added inline comments.
source/simulation2/components/CCmpUnitMotion.cpp
1193 ↗(On Diff #8496)

Can be moved at line 1214, right?

wraitii updated this revision to Diff 8783.Mon, Jul 8, 8:29 PM

Rebased for jenkins (maybe - might conflict)

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/1947/display/redirect

wraitii updated this revision to Diff 8804.Tue, Jul 9, 10:00 PM

This time it should patch and work correctly.

I'll probably add some tests before committing this

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/11/display/redirect

wraitii added inline comments.Wed, Jul 10, 10:41 AM
source/simulation2/components/CCmpUnitMotion.cpp
1269 ↗(On Diff #8804)

I think this bit here might be related to #5454 and I think it could safely be removed, I'll test.

wraitii added inline comments.Wed, Jul 10, 11:56 AM
source/simulation2/components/CCmpUnitMotion.cpp
1269 ↗(On Diff #8804)

er actually no because this is max range, not min range. Still it might be removed safely.

Rescinding the test comment, it's quite annoying to test a component right now, particularly when one wants to test things that aren't exposed to the interface. That _would_ require additional work which I might do later, but not right now.

source/simulation2/components/CCmpUnitMotion.cpp
1269 ↗(On Diff #8804)

As said on the ticket, turned out to be completely unrelated.
The reason why this was there - I believe - is that units aren't supposed to go to building edges directly since that's likely to make the long-range pathfinder choke. However since I'm now adding the clearance, it can safely be removed.

It was added in rP7484.

wraitii updated this revision to Diff 8820.Wed, Jul 10, 8:31 PM

Comment touch-ups.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/22/display/redirect

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Jul 10, 8:42 PM
This revision was automatically updated to reflect the committed changes.