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.

Sign up for GitHub

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

Jump to bottom

Decoupled cMonster and path recalc logic, re-implemented recalc. #2668

Merged
merged 1 commit into from Dec 13, 2015
Merged

Decoupled cMonster and path recalc logic, re-implemented recalc. #2668

merged 1 commit into from Dec 13, 2015

Conversation

SafwatHalaby
Copy link
Member

Do not merge yet. This needs review. This is in pretty good shape, but not yet complete.

Fixes #2536

Known issues:

  • Fixed, Mobs sometimes do not detect that they've reached their final destination. The result is that passive mobs sometimes just sit still.
  • Fixed, Needs to conform to the lua style check script.
  • Fixed, It appears I've reverted some commits made by others, need to fix that.
  • Fixed, Need to review and update any out-of-date AI-related doxy docs in the headers.

SafwatHalaby
SafwatHalaby reviewed Nov 20, 2015
View reviewed changes
src/Mobs/Monster.cpp
if (m_JumpCoolDown == 0)
{
if (DoesPosYRequireJump(FloorC(m_NextWayPointPosition.y)))
{
if (
(IsOnGround() && (GetSpeedX() == 0.0f) && (GetSpeedY() == 0.0f)) ||
(IsSwimming() && (m_GiveUpCounter < 15))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember touching these two lines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the fix to avoid taking fall damage while swimming in water
Nope, that was player, not mob

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO investigate this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Monster.cpp no longer has a GiveUpCounter, this is why I edited this line. It breaks jumping in water, but AI water handling is pretty broken altogether, so this isn't an issue. We'll get back to this when water handling is fixed.

@SafwatHalaby
Copy link
Member Author

Edit: Fixed
Clang error:

     conversion loses integer precision: 'unsigned long' to 'int'

      [-Werror,-Wshorten-64-to-32]

                return m_PathPoints.size() - m_CurrentPoint;

                ~~~~~~ ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~

SafwatHalaby
SafwatHalaby reviewed Nov 20, 2015
View reviewed changes
src/Mobs/PathFinder.cpp
}

// TODO fix monster heads
// TODO why does PATH_NOT_FOUND even occur?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Irrelevant TODO comments, already fixed in this commit

@tigerw
Copy link
Member

tigerw commented Nov 21, 2015

D'you know what? Why don't you just merge yourself after you've finished your own review? :P

NiLSPACE
NiLSPACE reviewed Nov 24, 2015
View reviewed changes
src/Mobs/Monster.cpp
}

if (ReachedFinalDestination())
if ((m_NextWayPointPosition - GetPosition()).Length() < GetWidth()/2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space around the /

Also, add extra parentheses to conditions. (GetWidth() / 2)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, the extra parenthesis is not really needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? The convention says that it needs to be like this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention says "parenthesis around comparisons", not "parenthesis around comparison operands"
Similarly, we write if (a > b + 2) instead of if (a > (b + 2))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't that bad? It looks awful

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lua script missed this one.

@SafwatHalaby SafwatHalaby mentioned this pull request Nov 25, 2015
Closed
@SafwatHalaby
Copy link
Member Author

It took me a while to realize that I'm actually using PathDestination for two distinct purposes. I split that into two variables and that solved most (or all?) issues.

@SafwatHalaby
Copy link
Member Author

Do not merge yet, even if CI succeeds.

@SafwatHalaby
Copy link
Member Author

Rebased.
Looking good, but I'm still not satisfied with idle mob behavior. I'll need to rethink a bit.

@SafwatHalaby
Copy link
Member Author

The last commit fixes idle mob issues.
I changed the PathFinder.h header a bit.
I Added a dontCare bool. If set to true, it means the monster doesn't care where it would end up going, and the PathFinder may change the monster's final destination.

@SafwatHalaby
Copy link
Member Author

Also, I used a pointer because it explicitly indicates that the Pathfinder may modify the finalDestination. WIth references, I found myself putting comments all over the place to prevent confusion.

@SafwatHalaby
Copy link
Member Author

lgtm. Rebased.

SafwatHalaby
SafwatHalaby reviewed Dec 12, 2015
View reviewed changes
src/Mobs/PathFinder.h
@param a_Destination The position the mob would like to reach. If a_ExactPath is true, the PathFinder may modify this.
@param a_OutputWaypoint An output parameter: The next waypoint to go to.
@param a_DontCare If true, the mob doesn't care where to go, and the Pathfinder may modify a_Destination.
This should usually be true. An exception is a wandering idle mob which doesn't care about its final destination.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should usually be false.

@Seadragon91
Copy link
Contributor

There are still a few TODOs. Will they be done in a other pull request, someday?

@SafwatHalaby
Copy link
Member Author

@Seadragon91 Which TODOs are you referring to?

@SafwatHalaby
Copy link
Member Author

@Seadragon91
Monster give up timer - No longer relevant
monster.h TODO - Done
goto TODO - Will do later

lgtm.

SafwatHalaby added a commit that referenced this pull request Dec 13, 2015
@SafwatHalaby
Merge pull request #2668 from SafwatHalaby/decouple2
86b5108 
Decoupled cMonster and path recalc logic, re-implemented recalc.
@SafwatHalaby SafwatHalaby merged commit 86b5108 into cuberite:master Dec 13, 2015
@SafwatHalaby SafwatHalaby deleted the decouple2 branch December 13, 2015 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Find a better path recalc algorithm
6 participants
@SafwatHalaby @tigerw @Seadragon91 @NiLSPACE @sphinxc0re @madmaxoft