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.

Sign up for GitHub

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

Jump to bottom

some improvement part 3 #532

Merged
2 commits merged into from
Aug 27, 2018
Merged

some improvement part 3 #532

2 commits merged into from
Aug 27, 2018

Conversation

mschultheiss83
Copy link
Contributor

fixes some routing and direction problems with carry and soucer

TooAngel
TooAngel requested changes Jul 12, 2018
View reviewed changes
src/prototype_creep_routing.js Outdated
console.log(`getDirections: pathPos: ${pathPos} pos: ${pos} path: ${JSON.stringify(path)}`);
throw e;
console.log(`${Game.time} getDirections: pathPos: ${pathPos} pos: ${pos} path: ${JSON.stringify(path)}`, e.stack);
currentPos = this.pos;
Copy link
Owner

Choose a reason for hiding this comment

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

Wondering why we not simply say: const currentPos = this.pos. Currently I'm missing why we get the pos from the path.
Not sure if it is just stupid implementation to set currentPos in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was wondering why you choose try catch and not just a const currentPos = this.pos; in the last update 😄

src/prototype_creep_routing.js Outdated
backwardDirection = currentPos.getDirectionTo(nextPos.x, nextPos.y);
}

if (currentPos.isEqualTo(nextPos)) {
Copy link
Owner

Choose a reason for hiding this comment

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

How can this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on transfer between rooms i thought

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

src/prototype_creep_routing.js Outdated
if (currentPos.isEqualTo(nextPos)) {
this.creepLog('getDirections: i need to stay');
}
if (!backwardDirection && this.memory.routing.reverse) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like this. I think we shouldn't mess around with it here. The caller of the method should handle the case, that we don't have the direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the easiest way to fix carry on the wrong side due to rooms skip.

src/prototype_creep_routing.js
let offset = 1;
if (this.memory.routing.reverse) {
offset = -1;
direction = backwardDirection;
} else {
this.memory.routing.reverse = false;
Copy link
Owner

Choose a reason for hiding this comment

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

Why? Isn't it anyway the case that this.memory.routing.rever is false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, some time it is null

Copy link
Owner

Choose a reason for hiding this comment

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

Don't think this is necessary the best place, but fine.

src/prototype_creep_routing.js Outdated
@@ -151,6 +159,7 @@ Creep.prototype.followPath = function(action) {
}

// Recalculate the directions, if `preMove` changed `memory.routing.reversed`
this.memory.routing.pathPos = this.getPathPos(path);
Copy link
Owner

Choose a reason for hiding this comment

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

this.getPathPos(path) updates this.memory.routing.pathPos already, so this.getPathPos(path); is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok changed

src/prototype_creep_routing.js Outdated
if (config.debug.routing) {
this.log(`${Game.time} moveByPathMy: Directions invalid pathPos: ${pathPos} path[pathPos]: ${path[pathPos]} directions: ${global.ex(directions, 1)}`);
}
return true;
Copy link
Owner

Choose a reason for hiding this comment

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

Why returning true? We don't move we should tell that to the caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because you return in followPath (prototype_creep_routing.js:175) moveByPathMy, and that leads to unnessesary 'Directions invalid messages' for carries...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because of the skipped rooms they are on the wrong room side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so it lead to 'Reached end of handling() why?' for carries and some scourcers

@TooAngel
Copy link
Owner

Please fix the codeclimate issues, I improved the rating to A and we should try to stay there.

Will try to bring the other files to a similar level, but for the ones I did, I would like to force that codeclimate needs to be green, too.

@mschultheiss83
Copy link
Contributor Author

btw in moveByPathMy we return moveBackToPath.
moveBackToPath return a number, and we want boolean,
should we return this.moveBackToPath(path) > 0; instead?

@TooAngel
Copy link
Owner

btw in moveByPathMy we return moveBackToPath.
moveBackToPath return a number, and we want boolean,
should we return this.moveBackToPath(path) > 0; instead?

Hm, good question, I'm not sure yet, we can start with > 0.

@TooAngel
Copy link
Owner

Same here codeclimate for prototype_creep_routing and I'm fine

@TooAngel
Copy link
Owner

@mschultheiss83 Can you please fix the codeclimate issues (https://codeclimate.com/github/TooAngel/screeps/pull/532) in src/prototype_creep_routing.js and I'm fine.

@mschultheiss83
Copy link
Contributor Author

I'm on holiday. I can try in 3 Weeks

@mschultheiss83
Copy link
Contributor Author

everything is fixed in #533

@mschultheiss83
Copy link
Contributor Author

except in https://codeclimate.com/github/TooAngel/screeps/pull/533 moveByPathMy has a complexity of 7

@mschultheiss83 mschultheiss83 mentioned this pull request Aug 20, 2018
Merged
@ghost
Copy link

ghost commented Aug 22, 2018

A new review, yeah.

        Votes: 52/617
        Coefficient: 0.08427876823338736
        Merging in 11 days 4.863333333333333 hours
        Age 2 days 12.796111111111111 hours

@ghost ghost merged commit 0e69a7b into TooAngel:master Aug 27, 2018
@mschultheiss83 mschultheiss83 deleted the feature/routing-direction branch February 25, 2023 15:06
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TooAngel TooAngel TooAngel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mschultheiss83 @TooAngel