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

Doors #1445

Closed
wants to merge 13 commits into from
Closed

Doors #1445

wants to merge 13 commits into from

Conversation

powerboat9
Copy link

I wrote some code to handle doors better. Now, baritone can walk through iron doors that aren't obstructing its path. This should also make it much easier to handle trapdoors.

@5HT2 5HT2 requested a review from leijurv March 25, 2020 17:54
@5HT2 5HT2 added enhancement New feature or request good first issue Good for newcomers labels Mar 25, 2020
Copy link
Contributor

@5HT2 5HT2 left a comment

Choose a reason for hiding this comment

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

Clean solution, coding style is good. lgtm 👍

@powerboat9
Copy link
Author

It doesn't like going right->left diagonally with this
2020-03-25_15 01 26
I'll fix it before we merge

@powerboat9
Copy link
Author

Ok, the bugs should be ironed out and it's passing all the checks (except Codacy's npath requirements, but I'm not sure if those are actual requirements).

@leijurv
Copy link
Member

leijurv commented Mar 26, 2020

Thank you so much for taking the time to dive in and make this PR :)

Looks pretty cool. Codacy doesn't really matter as long as it's formatted right, which it is.

Here's the thing though: the pathing thread is, as you might imagine, performance-dominated by movement cost calculation. All the other A* stuff like heuristic, open set heap, and node map end up contributing very little to the performance.

A big part of why baritone is so fast is something I did pretty early on, here #180 which is that movement cost calculation is not allowed to allocate any objects. Not even a single new BlockPos, not even indirectly such as BlockPos.up(). It previously was creating tens of millions of junk objects per second, because, remember, Baritone zooms. The budget for how long a movement cost calculation is allowed to take is measured in the dozens to low hundreds of nanoseconds (yes, seriously, nanoseconds). For that reason, I can't accept this as-is because of how it creates new SpaceRequest objects many times per movement cost calculation. Sorry!

@leijurv
Copy link
Member

leijurv commented Mar 26, 2020

😻 holy cr*p

@leijurv
Copy link
Member

leijurv commented Mar 26, 2020

image

@leijurv
Copy link
Member

leijurv commented Mar 26, 2020

im having a really hard time figuring out what the heck is supposed to be happening here?

it looks to my eyes this will double the cost of a traverse where the source and destination are currently underground, such as during auto mining? because you're now also taking into account the cost of mining the source blocks?? why?

we assume that by the time it's this movement's turn to run, the previous movement has gotten our player into that position, so those blocks will have already been cleared

also seems like a missed opportunity to do all this for doors and not also for, e.g., fence gates and trapdoors...

@powerboat9
Copy link
Author

It checks the source blocks in order to make sure that the player isn't standing in the same block as a door which would block the player's movement.

EX:
2020-03-25_22 22 29

also seems like a missed opportunity to do all this for doors and not also for, e.g., fence gates and trapdoors...

That's what I want to work on next, but I thought it would be better to make that a separate pull request.

Efficiency could be improved further, so I guess I'll look into that

@powerboat9
Copy link
Author

This may be over complicated, but perhaps multiple heuristics functions could be used? You'd keep the current heuristic, but afterwards you could have a heuristic that works using only PathingBlockType data (AIR, WATER, AVOID, SOLID), and then finally use the regular calculations.

@5HT2 5HT2 mentioned this pull request Mar 26, 2020
2 tasks
@5HT2
Copy link
Contributor

5HT2 commented Mar 26, 2020

This closes #580 and I think it closes #389 as well.

@powerboat9
Copy link
Author

I don't think it fixes #389, but it does fix #580

@leijurv
Copy link
Member

leijurv commented Mar 26, 2020

Right, but your code is adding in the cost of breaking the source blocks. This is incorrect.

@5HT2
Copy link
Contributor

5HT2 commented Mar 26, 2020

Also closes #1454, for reference.

@powerboat9
Copy link
Author

It doesn't actually fix #1454 though. I'm trying to solve one thing at a time, and will probably fix #1454 at some point, but I thought I'd make sure it was recorded as an issue.

@powerboat9
Copy link
Author

Only MovementTraverse actually tries to open doors and fence gates, and you'd have to create a few utility methods and refactor some stuff in order to get other movements to open doors and fence gates.

@powerboat9
Copy link
Author

Right, but your code is adding in the cost of breaking the source blocks. This is incorrect.

Oh, that's if the player is standing inside an iron door

@powerboat9
Copy link
Author

Most of getMiningDurationTicks() is skipped if the player can walk through the block they're standing on.

@powerboat9
Copy link
Author

Hang on

@leijurv
Copy link
Member

leijurv commented Mar 26, 2020

No, traverse is called underground quite commonly. You aren't supposed to include the cost of mining the source blocks!

@powerboat9
Copy link
Author

This should be faster

Copy link
Contributor

@5HT2 5HT2 left a comment

Choose a reason for hiding this comment

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

This looks a lot better 👍

@powerboat9
Copy link
Author

powerboat9 commented Mar 27, 2020

2020-03-26_21 07 17
It tries to break the iron door despite the increased time required to break it, but that kind of situation should pop up too infrequently to be a problem.

@powerboat9
Copy link
Author

It works normally top->bottom though

@powerboat9
Copy link
Author

Can it be merged, or does it need improvement?

@5HT2 5HT2 requested review from leijurv and removed request for leijurv March 27, 2020 23:09
if (totalCost >= COST_INF) {
return;
}
totalCost += MovementHelper.getMiningDurationTicks(context, destX, y, destZ, false);
totalCost += MovementHelper.getMiningDurationTicks(context, x, y, z, false, SpaceRequest.fromFaces(direction));
Copy link
Member

@leijurv leijurv Aug 17, 2020

Choose a reason for hiding this comment

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

Sorry. We just can't do this. This double counts ALL cost.

If there's a traverse for example deep underground, this would count the cost of breaking the starting position (two blocks) and the cost of breaking the destination position (two blocks). That just isn't correct. The core movement logic assumes that by the time a movement starts, the previous movement is completed and has moved the player to the start of this movement, which includes breaking any blocks that may have been in the way. This breaks that assumption for the sole benefit of allowing us to open doors that we're standing in at the beginning of the path. There are better ways to accomplish that that don't involve breaking cost calculation invariants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants