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

Add new "Knight" movement option #3014

Closed
wants to merge 1 commit into from

Conversation

ShadowJonathan
Copy link

Modelled after the diagonal movement option, this allows 2 "long" and 1 "side", allowing smoother movement instead of constantly going between diagonal<->linear.

Visualisation for y'all;

image

This bumps into (touchable) corners when they're able to walked into, not sure if that's a thing y'all want.

This doesn't go down or up one level, it's more intended to be nice when traversing a large platform somewhat diagonally.

This is probably un-optimised as heck.

Copy link
Collaborator

@ZacSharp ZacSharp left a comment

Choose a reason for hiding this comment

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

I have done a quick review, but I'm not sure if this should be merged at all, because I don't see how it could enable Baritone to use paths it could not use before.
If there are no new possible paths this is simply post smoothing during pathing, which - as the name suggests - is better done once the path is found. Proper post smoothing instead of a new movement could also allow for flat moves in any angle without any additional calculation on the pathing thread. The disadvantage of post smoothing would be that it would consider a constant igzag of 2 blocks long segments as as good as a straight diagonal line, but imo that is better dealt with by changing to theta* rather than by trying to cover all diagonal lines (a lot) with their own movement.

Comment on lines +124 to +129
public static void cost(CalculationContext context, int srcX, int y, int srcZ, int dstX, int dstZ, MutableMoveResult res) {
if (!MovementHelper.canWalkThrough(context.bsi, dstX, y, dstZ)
|| !MovementHelper.canWalkThrough(context.bsi, dstX, y + 1, dstZ)
|| !MovementHelper.canWalkOn(context.bsi, dstX, y - 1, dstZ)) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This neither allows breaking blocks in the way nor placing blocks to walk on?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah it's quite a simple check :/

I'll have to add it before this'd be approved, then

return;
}

KnightDirection dir = new KnightDirection(dstX - srcX, dstZ - srcZ);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Object allocation is forbidden in cost calculation.
See #179, #180, #1445 (comment) and #2142 (comment)

I won't search out and mark every single spot where you create an object here, because you do it quite a lot.

@ShadowJonathan
Copy link
Author

Hmmm, this was indeed intended to be somewhat of a "smoothening" move, but your recommendation is to build in a specialised smoothing mechanism + switch to another algorithm for that?

I'd be interested in learning for it to accomplish that, it'd be interesting.

@ShadowJonathan
Copy link
Author

ShadowJonathan commented Oct 3, 2021

Do you have a IRC Nick/telegram account? I'd like to discuss this in-depth, and maybe work towards implementing this

@ShadowJonathan ShadowJonathan marked this pull request as draft October 3, 2021 12:24
@ZacSharp
Copy link
Collaborator

ZacSharp commented Oct 8, 2021

I definitely think post-smoothing would be a good addition, because it can significantly improve path lengths and make Baritone less painful to watch, without adding any overhead to the pathfinder itself.

I'm not sure on whether you should try switching Baritone to theta* (which is essentially a smarter way of post smoothing while pathing) because optimal algorithms are nice (and theta* isn't while a* can be) and because it adds significant overhead for all of the line of sight checks, while one of the first things mentioned in the readme is "Baritone focuses on reliability and particularly performance (it's over 30x faster than MineBot at calculating paths)."
Another option I'm not sure about is the multi-destination movements done for a (sadly closed) parkour pr, which would not require switching algorithms and could maybe maybe be a bit faster because it can be smarter about what to check.

My personal favourite currently is Anya, because it is pretty much the only optimal any-angle path finding algorithm I know and because it is mostly even faster than a*, but even though I haven't looked into it yet I have the feeling we can't use Anya in Baritone, just like a dozen other common performance improvement methods (most of them are for unweighted graphs or rely on blocked edges which Baritone does not have).

Also I'm a bit hesitant to give advice on such fundamental changes because after all it's leijurvs project and I don't want you do put a lot of effort into just to be told that it won't be used.

EDIT: yes, I'm ZacSharp on libera.chat

@ShadowJonathan
Copy link
Author

ShadowJonathan commented Oct 11, 2021

@ZacSharp you're not on libera when i checked just now, do you have any other networks you're on 24/7? (I'm ShadowJonathan on Libera)

Also I'm a bit hesitant to give advice on such fundamental changes because after all it's leijurvs project and I don't want you do put a lot of effort into just to be told that it won't be used.

Yeah i checked in with them on this, and i got a denial to switch away from A*. I think post-smoothening would be the way to go here, though im not exactly sure how to go about doing that.

@scorbett123
Copy link
Collaborator

Libera has a major issue of not saving previous messages, it means that you have to both be online to send messages, is there another massaging service that @ZacSharp will use that can save messages?

@ShadowJonathan
Copy link
Author

I don't believe this'll go anywhere, so i'll close this but keep the branch up.

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.

3 participants