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.

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

Cleaner Creature follow logic #4634

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

danilopucci
Copy link
Contributor

Pull Request Prelude

This PR adds small changes on Creature Follow logic by renaming and moving some code. The changes here does not affect any logic to avoid introducing any kind of bug.

Changes Proposed

Small improvement on Creature Follow logic by:

  • changes on Creature::getPathSearchParams

    • rename it to Creature::buildFindPathParams as the object name that it is building is FindPathParams
    • remove abbreviation on its parameter (and also from the callers)
    • add a new parameter which sets fullSearchPath
  • changes on Creature::goToFollowCreature:

    • its callback was renamed from onFollowCreatureComplete to onGoToFollowCreatureComplete
    • Monster logic was moved to proper Monster::goToFollowCreature override
    • repeated code was moved to a new method Creature::updateFollowPath

I splitted the mentioned changes on different commits, so if the reviewers wants to look into small pieces of changes, it is easier to look on each commit.

It is worth to say that proabably there is more logic that can be cleaned and simplified, but I avoid to do that now just to be a safer change.

Issues addressed:

EPuncker
EPuncker previously approved these changes Mar 14, 2024
@MillhioreBT
Copy link
Contributor

I didn't understand why you changed the short name of the fpp variable to findPathParams XD
but ok..

@4mrcn4
Copy link
Contributor

4mrcn4 commented Mar 14, 2024

I didn't understand why you changed the short name of the fpp variable to findPathParams XD but ok..

In time you will learn that letternamed variables does not save time, they waste it.

@MillhioreBT
Copy link
Contributor

I didn't understand why you changed the short name of the fpp variable to findPathParams XD but ok..

In time you will learn that letternamed variables does not save time, they waste it.

No one has said anything about that... wtf

I'm just saying that sometimes people love to modify unnecessary things, how ugly the whole name repeated everywhere looks, as if that change were necessary, it's silly to change the name of the variable, it will only add noise to the PR

@danilopucci
Copy link
Contributor Author

Yeah, as @4mrcn4 pointed out, I renamed it to be more readable and meaningfull to what the variable represents. When I was first reading it, it took me some time to remember what fpp was :p

I 100% agree on @MillhioreBT thoughs on avoid change unecessary things and bring noise to the PR, but I honestly do not think that is the case, as the variable name changes are related to the method that I changed and on the diff is pretty clear that it is a single renaming.

@MillhioreBT
Copy link
Contributor

MillhioreBT commented Mar 15, 2024

Yeah, as @4mrcn4 pointed out, I renamed it to be more readable and meaningfull to what the variable represents. When I was first reading it, it took me some time to remember what fpp was :p

I 100% agree on @MillhioreBT thoughs on avoid change unecessary things and bring noise to the PR, but I honestly do not think that is the case, as the variable name changes are related to the method that I changed and on the diff is pretty clear that it is a single renaming.

How many hours did it take you to identify what fpp that means?
image
XD

I love that everyone supports the project and adds things, so it was just a healthy comment, thanks for the changes <3

fix the code format and I will test it and then approve it if everything goes well.

@danilopucci
Copy link
Contributor Author

@MillhioreBT Thank you so much

I have just fixed the code style

@danilopucci
Copy link
Contributor Author

@MillhioreBT did you had any time to test it?

src/creature.cpp Outdated Show resolved Hide resolved
Co-authored-by: Marcin Michalski <evulmastah@gmail.com>
@MillhioreBT
Copy link
Contributor

@MillhioreBT did you had any time to test it?

Please leave the variable name as it was originally, just make the necessary logic changes to clean up the logic.

@danilopucci
Copy link
Contributor Author

danilopucci commented Mar 25, 2024

Please leave the variable name as it was originally, just make the necessary logic changes to clean up the logic.

I reverted the findPathParams to fpp, can you please take a look again? @MillhioreBT

@danilopucci
Copy link
Contributor Author

Hey @MillhioreBT, did you had time to test it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants