otland / forgottenserver

A free and open-source MMORPG server emulator written in C++
https://otland.net
GNU General Public License v2.0
1.57k stars 1.05k forks source link

Cleaner Creature follow logic #4634

Open danilopucci opened 5 months ago

danilopucci commented 5 months ago

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:

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:

MillhioreBT commented 5 months ago

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

marcingoralski commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

@MillhioreBT Thank you so much

I have just fixed the code style

danilopucci commented 5 months ago

@MillhioreBT did you had any time to test it?

MillhioreBT commented 5 months ago

@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 commented 5 months ago

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 commented 4 months ago

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