Skip to content

Conversation

@Toffikk
Copy link

@Toffikk Toffikk commented Oct 16, 2025

Fixes #13191

@Toffikk Toffikk requested a review from a team as a code owner October 16, 2025 15:50
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Oct 16, 2025
@Toffikk Toffikk changed the title feat: Add a new overload for Pathfinder#findPath and deprecate the old method for removal feat: Add a new overloads for Pathfinder and deprecate the old methods for removal Oct 16, 2025
@Warriorrrr
Copy link
Member

Not really a fan of deprecating the existing method, wouldn't that require everyone to cast to Entity to not get the deprecation warning? The existing method could also become a default and call the new one.

@Toffikk Toffikk force-pushed the feat/pathfinder-overload branch from 1e557ab to 9e90a83 Compare October 16, 2025 15:58
@Toffikk
Copy link
Author

Toffikk commented Oct 16, 2025

Not really a fan of deprecating the existing method, wouldn't that require everyone to cast to Entity to not get the deprecation warning? The existing method could also become a default and call the new one.

I get the point but i deprecated it because LivingEntity is extending Entity so we shouldn't keep an overload that's basically going to do the same thing as the other one but more limited, the best solution would be to remove the old one but that would cause all plugins to need to be recompiled. The devs can choose to either suppress that warning in some way or just wait until the old one gets removed and update their dependency on paper-api and everything will work fine, no matter if they start casting to Entity now or leave their code as it is

@Toffikk Toffikk changed the title feat: Add a new overloads for Pathfinder and deprecate the old methods for removal feat: Add new overloads for Pathfinder and deprecate the old methods for removal Oct 16, 2025
@Toffikk
Copy link
Author

Toffikk commented Oct 16, 2025

We could perhaps add an @apiNote with a notice that their plugins might require a recompile on the next version and a link to the new overloads

@masmc05
Copy link
Contributor

masmc05 commented Oct 16, 2025

so we shouldn't keep an overload that's basically going to do the same thing as the other one but more limited

I don't see why not, it's not like that default LivingEntity method would ever require maintenance when it's just redirecting to the Entity method

We could perhaps add an @APinote with a notice that their plugins might require a recompile on the next version and a link to the new overloads

Sounds extremely awful

@Toffikk
Copy link
Author

Toffikk commented Oct 16, 2025

so we shouldn't keep an overload that's basically going to do the same thing as the other one but more limited

I don't see why not, it's not like that default LivingEntity method would ever require maintenance when it's just redirecting to the Entity method

I'll change that then to redirect to the new one

@Toffikk Toffikk force-pushed the feat/pathfinder-overload branch from 4df47e0 to 15f61df Compare October 16, 2025 16:22
@Toffikk
Copy link
Author

Toffikk commented Oct 16, 2025

done, now the LivingEntity overloads just redirect to the Entity ones

@Toffikk Toffikk changed the title feat: Add new overloads for Pathfinder and deprecate the old methods for removal feat: Add new overloads for Pathfinder Oct 16, 2025
@Toffikk Toffikk force-pushed the feat/pathfinder-overload branch from f9a599c to a91cc3d Compare October 17, 2025 23:45
@Toffikk Toffikk force-pushed the feat/pathfinder-overload branch from a91cc3d to 29bfd67 Compare October 17, 2025 23:47
@Toffikk Toffikk requested a review from Doc94 October 17, 2025 23:49
Copy link
Member

@Doc94 Doc94 left a comment

Choose a reason for hiding this comment

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

also i found another javadoc issues not caused by this PR but maybe can be good fix like L185

@github-project-automation github-project-automation bot moved this from Awaiting review to Changes required in Paper PR Queue Oct 18, 2025
@Toffikk Toffikk requested a review from Doc94 October 18, 2025 00:39
/**
* @return Returns the index of the current point along the points returned in {@link #getPoints()} the entity
* is trying to reach. This value will be higher than the maximum index of {@link #getPoints()} if this path finding is done.
* is trying to reach. This value will be higher than the maximum index of {@link #getPoints()} if this path finding is done
Copy link
Contributor

Choose a reason for hiding this comment

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

That's wrong as per oracle convention, also i would rather not do cleanup of this nature until checkstyle is in, can you revert all your changes apart from the two overload changes.

@Toffikk
Copy link
Author

Toffikk commented Oct 27, 2025

@Lulu13022002 everything should be addressed now👍
I left the repeated to removal tho

@Toffikk Toffikk requested a review from Lulu13022002 October 27, 2025 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Changes required

Development

Successfully merging this pull request may close these issues.

Add Pathfinder#moveTo(Entity)

5 participants