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 4 #533

Merged
5 commits merged into from
Feb 11, 2019
Merged

some improvement part 4 #533

5 commits merged into from
Feb 11, 2019

Conversation

mschultheiss83
Copy link
Contributor

added pickupEnergy in getEnergy
better sellByOthersOrders
fixed canHelpRooms + needEnergyRooms
fixed energy and power transfer of role_storagefiller

TooAngel
TooAngel requested changes Jul 12, 2018
View reviewed changes
src/config.js Outdated
@@ -112,7 +112,7 @@ global.config = {
maxRooms: 8,
cpuPerRoom: 13, // Necessary CPU per room, prevent claiming new rooms
revive: true,
maxDistance: 17,
maxDistance: 10,
Copy link
Owner

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my creepy died before they reach 17, so why seach for far away places, that aren't rechable

Copy link
Owner

Choose a reason for hiding this comment

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

Mind to write this as a comment to the line, so that we know in the future why we went in this direction?

src/config.js Outdated
minEnergyAmount: 80000,
maxEnergyAmount: 120000,
minEnergyAmount: 40000,
maxEnergyAmount: 50000,
Copy link
Owner

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my terminal we full with energy at 80k+ and i had no space

Copy link
Owner

Choose a reason for hiding this comment

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

Mind to write this as a comment to the line, so that we know in the future why we went in this direction?

src/config.js Outdated
@@ -242,7 +242,7 @@ global.config = {
},

market: {
minAmountToSell: 100000,
minAmountToSell: 50000,
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, I think we should explain why we set a specific value / change to a specific value in the config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here, my terminal we full with energy at 80k+ and i had no space

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 only energy at 😄

Copy link
Owner

Choose a reason for hiding this comment

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

Mind to write this as a comment to the line, so that we know in the future why we went in this direction?

src/prototype_creep_startup_tasks.js Outdated
@@ -117,6 +117,7 @@ Creep.recycleCreep = function(creep) {
};

Creep.getEnergy = function(creep) {
creep.pickupEnergy();
Copy link
Owner

Choose a reason for hiding this comment

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

If we want to add that, it should be in Creep.prototype.getEnergy and not hidden in the abstraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will be removed in next PR. the hidden abstraction we dont need.
i dont like Creep.getEnergy and that method.push stuff.

Copy link
Owner

Choose a reason for hiding this comment

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

hm, if you touch the PR anyway, I would suggest to remove it, if it get's anyway removed.

src/prototype_room_market.js Outdated
(this.terminal.store[RESOURCE_ENERGY] > config.terminal.minEnergyAmount)) {
const diffNeedEnergyRooms = _.difference(Memory.needEnergyRooms, Memory.canHelpRooms);
const needEnergyRooms = _.size(diffNeedEnergyRooms) > 0 ? diffNeedEnergyRooms : Memory.needEnergyRooms;
const myRoom = _.shuffle(needEnergyRooms)[0];
Copy link
Owner

Choose a reason for hiding this comment

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

Why not send to the closest one? It's cheaper

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 did that before, but than not all rooms get the energy they need, only the closest to the best farming, and i wannted to push as much in the controllers of all rooms as i could

src/prototype_room_market.js Outdated
_.includes(Memory.canHelpRooms, this.name) && !_.includes(Memory.needEnergyRooms, this.name) &&
(this.terminal.store[RESOURCE_ENERGY] > config.terminal.minEnergyAmount)) {
const diffNeedEnergyRooms = _.difference(Memory.needEnergyRooms, Memory.canHelpRooms);
const needEnergyRooms = _.size(diffNeedEnergyRooms) > 0 ? diffNeedEnergyRooms : Memory.needEnergyRooms;
Copy link
Owner

Choose a reason for hiding this comment

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

To be honest I wouldn't to that. We should rely on, that Memory.needEnergyRooms and the ones which actually need energy. (Add they shouldn't be in the other list)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean !_.includes(Memory.needEnergyRooms, this.name) &&
as i was working on the improvements, it did happen some times... i will remove

src/prototype_room_market.js Outdated
Room.prototype.handleMarket = function() {
if (!this.terminal) {
if (!this.terminal || this.terminal.cooldown || this.terminal.cooldown > 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

if (!this.terminal || this.terminal.cooldown) should be fine, if this.terminal.cooldown is 0, it is a 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.

sure, changed

@mschultheiss83
getEnergy + pickupEnergy, sellByOthersOrders, canHelpRooms + needEner…
4d3d17a 
…gyRooms, role_storagefiller

added tooangel comments + improvements
@mschultheiss83 mschultheiss83 mentioned this pull request Aug 20, 2018
Merged
@mschultheiss83
commented config and removed getEnergy
f06bbf6 
added missing comment
make codeclimate happy
@mschultheiss83
Copy link
Contributor Author

should merged after #532

TooAngel
TooAngel approved these changes Aug 26, 2018
View reviewed changes
src/prototype_creep_startup_tasks.js
@@ -125,18 +125,13 @@ Creep.repairStructure = function(creep) {
};

Creep.prototype.getEnergyFromHostileStructures = function() {
if (this.carry.energy) {
return false;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I would keep it like this. If the creep has energy we spare a find() call = cpu usage

@ghost
Copy link

ghost commented Aug 26, 2018

A new review, yeah.

        Votes: 52/617
        Coefficient: 0.08427876823338736
        Merging in 12 days 0.17444444444444446 hours
        Age 6 days 7.371388888888889 hours

@mschultheiss83
Merge branch 'master' into feature/cash-energy-flow
b79dc23 
# Conflicts:
#	src/prototype_creep_resources.js
#	src/prototype_creep_routing.js
@mschultheiss83 mschultheiss83 changed the title some improvement part 4 [WIP] some improvement part 4 Sep 4, 2018
@mschultheiss83
Copy link
Contributor Author

please doubble check

@mschultheiss83
Copy link
Contributor Author

Added WIP until FIND_TOMBSTONES gets fixed

   src/prototype_creep_resources.js
    299 |  const tombstones = this.pos.findInRange(FIND_TOMBSTONES, 1);
                                                   ^ 'FIND_TOMBSTONES' is not defined.

@ghost ghost added the WIP label Sep 4, 2018
@mschultheiss83 mschultheiss83 changed the title [WIP] some improvement part 4 some improvement part 4 Feb 11, 2019
@ghost ghost removed the WIP label Feb 11, 2019
@ghost ghost merged commit 5dc8d9d into TooAngel:master Feb 11, 2019
@mschultheiss83 mschultheiss83 deleted the feature/cash-energy-flow branch February 25, 2023 15:07
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