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

Spawn.spawnCreep(body, name, options) #38

Merged
merged 23 commits into from Sep 26, 2017

Conversation

Projects
None yet
8 participants
@Creosteanu
Contributor

Creosteanu commented Jun 19, 2017

Options: {memory, energyStructures}

Allows specifying which energyStructures (spawn/extensions) to use for drawing energy.

@ButAds

This comment has been minimized.

Show comment
Hide comment
@ButAds

ButAds Jun 19, 2017

Contributor

Great addition. It has been requested quite a few times.

http://support.screeps.com/hc/en-us/community/posts/211747065--StructureSpawn-New-argument-in-createCreep

The way it's implemented it provides backwards compatibility to old code as well.

Contributor

ButAds commented Jun 19, 2017

Great addition. It has been requested quite a few times.

http://support.screeps.com/hc/en-us/community/posts/211747065--StructureSpawn-New-argument-in-createCreep

The way it's implemented it provides backwards compatibility to old code as well.

@ButAds

ButAds approved these changes Jun 19, 2017

@DoctorPCfin

This comment has been minimized.

Show comment
Hide comment
@DoctorPCfin

DoctorPCfin Jun 20, 2017

A must-have feature that would allow many people to use their extensions in a much smarter way. For me personally, it would save up to 5 CPU per creep spawn in the refilling code.

DoctorPCfin commented Jun 20, 2017

A must-have feature that would allow many people to use their extensions in a much smarter way. For me personally, it would save up to 5 CPU per creep spawn in the refilling code.

@artch

This comment has been minimized.

Show comment
Hide comment
@artch

artch Jun 20, 2017

Contributor

OK, this seems to be a nice feature. Some comments:

  1. I don't see the need to create and process another type of intent. It can be just a conditional modification of the old intent's behavior, i.e. both spawnCreep and createCreep methods would send the same intent.

  2. Only structure IDs should be stored in the intent data, not objects. utils.storeIntents should serialize it into a concatenated string.

  3. We may add new option dryRun here which deprecates the canCreateCreep method. If dryRun is true, then spawnCreep simply does all checks and returns the code, but doesn't generate the intent. Thus, supporting energyStructures param in canCreateCreep is not neccessary anymore, since it would be simply marked as deprecated.

Has this PR been tested and how?

Contributor

artch commented Jun 20, 2017

OK, this seems to be a nice feature. Some comments:

  1. I don't see the need to create and process another type of intent. It can be just a conditional modification of the old intent's behavior, i.e. both spawnCreep and createCreep methods would send the same intent.

  2. Only structure IDs should be stored in the intent data, not objects. utils.storeIntents should serialize it into a concatenated string.

  3. We may add new option dryRun here which deprecates the canCreateCreep method. If dryRun is true, then spawnCreep simply does all checks and returns the code, but doesn't generate the intent. Thus, supporting energyStructures param in canCreateCreep is not neccessary anymore, since it would be simply marked as deprecated.

Has this PR been tested and how?

@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Jun 20, 2017

Contributor

Hi Arch,

Thanks for the feedback. I will apply the suggestions from 1,2 and 3.

The PR has not yet been tested. I plan on deploying it to the private server currently scheduled for the second edition of the warfare championship. I'll do so after the latest adjustments.

We should have some concrete feedback on its functionality in a day or two.

Contributor

Creosteanu commented Jun 20, 2017

Hi Arch,

Thanks for the feedback. I will apply the suggestions from 1,2 and 3.

The PR has not yet been tested. I plan on deploying it to the private server currently scheduled for the second edition of the warfare championship. I'll do so after the latest adjustments.

We should have some concrete feedback on its functionality in a day or two.

@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Jun 20, 2017

Contributor

The code has been updated according to the suggestions. I'll run it on a private server shortly and confirm that it works.

Contributor

Creosteanu commented Jun 20, 2017

The code has been updated according to the suggestions. I'll run it on a private server shortly and confirm that it works.

@ButAds

This comment has been minimized.

Show comment
Hide comment
@ButAds

ButAds Jun 22, 2017

Contributor

Any update on the testing on a private server?

Contributor

ButAds commented Jun 22, 2017

Any update on the testing on a private server?

@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Jun 23, 2017

Contributor

Apologies. The current world events have kept me busy. I'll try to get to this soon.

Contributor

Creosteanu commented Jun 23, 2017

Apologies. The current world events have kept me busy. I'll try to get to this soon.

@ButAds

This comment has been minimized.

Show comment
Hide comment
@ButAds

ButAds Jul 26, 2017

Contributor

How's the testing going for this? I'd love to see this in one of the next releases

Contributor

ButAds commented Jul 26, 2017

How's the testing going for this? I'd love to see this in one of the next releases

@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Jul 27, 2017

Contributor
Contributor

Creosteanu commented Jul 27, 2017

@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Aug 28, 2017

Contributor

@ButAds with things finally cooling down I got around to putting this stuff on a server and testing it. There were a few important fixes that I had to apply, but the feature now seems stable.

I have a server running on 35.156.32.205 with the new function. Feel free to drop by and play around.

Contributor

Creosteanu commented Aug 28, 2017

@ButAds with things finally cooling down I got around to putting this stuff on a server and testing it. There were a few important fixes that I had to apply, but the feature now seems stable.

I have a server running on 35.156.32.205 with the new function. Feel free to drop by and play around.

@ButAds

Looks good! I'm excited for this feature. Added a little bit of feedback.

Show outdated Hide outdated src/game/structures.js Outdated
Show outdated Hide outdated src/processor/intents/spawns/_charge-energy.js Outdated
Show outdated Hide outdated src/processor/intents/spawns/_charge-energy.js Outdated
@@ -23,13 +41,13 @@ module.exports = function(spawn, roomObjects, cost, bulk, roomController) {
cost -= neededEnergy;
bulk.update(i, {energy: i.energy});
});
if(cost <= 0) {
return true;
}
extensions.sort(utils.comparatorDistance(spawn));

This comment has been minimized.

@ButAds

ButAds Aug 29, 2017

Contributor

Is sorting still needed if energyStructures is set?

@ButAds

ButAds Aug 29, 2017

Contributor

Is sorting still needed if energyStructures is set?

This comment has been minimized.

@Creosteanu

Creosteanu Aug 30, 2017

Contributor

Good question. As above, my desire was to minimize the footprint of my changes and group them in an isolated way.

This one is more of a gameplay decision that @artch should take. I suppose if the player has specified the structures to use, the sort makes less sense.

@Creosteanu

Creosteanu Aug 30, 2017

Contributor

Good question. As above, my desire was to minimize the footprint of my changes and group them in an isolated way.

This one is more of a gameplay decision that @artch should take. I suppose if the player has specified the structures to use, the sort makes less sense.

This comment has been minimized.

@o4kapuk

o4kapuk Sep 8, 2017

Contributor

I don't think that sorting is needed here. This feature supposed to be an advanced mechanics for experienced players, and they'd like an opportunity to specify discharge order.

@o4kapuk

o4kapuk Sep 8, 2017

Contributor

I don't think that sorting is needed here. This feature supposed to be an advanced mechanics for experienced players, and they'd like an opportunity to specify discharge order.

This comment has been minimized.

@artch

artch Sep 9, 2017

Contributor

We can do it this way: if energyStructures is undefined, then sort order is by default using distance, otherwise energyStructures array order is used.

@artch

artch Sep 9, 2017

Contributor

We can do it this way: if energyStructures is undefined, then sort order is by default using distance, otherwise energyStructures array order is used.

This comment has been minimized.

@o4kapuk

o4kapuk Sep 9, 2017

Contributor

That would be the standard createCreep behavior, right?
Maybe we could extend createCreep itself with the 4th optional parameter (object which may contain energyStructures) instead of introducing new api method?

@o4kapuk

o4kapuk Sep 9, 2017

Contributor

That would be the standard createCreep behavior, right?
Maybe we could extend createCreep itself with the 4th optional parameter (object which may contain energyStructures) instead of introducing new api method?

This comment has been minimized.

@Creosteanu

Creosteanu Sep 18, 2017

Contributor

@o4kapuk the idea was to not change the existing createCreep and deprecate the method over a long period of time. Supposing of course that spawnCreep is gobbled up by the community.

I'll make the adjustment to only sort if energyStructures is undefined.

@Creosteanu

Creosteanu Sep 18, 2017

Contributor

@o4kapuk the idea was to not change the existing createCreep and deprecate the method over a long period of time. Supposing of course that spawnCreep is gobbled up by the community.

I'll make the adjustment to only sort if energyStructures is undefined.

This comment has been minimized.

@NobodysNightmare

NobodysNightmare Sep 29, 2017

I am not super familiar with the code. Does that mean I could always provide all extensions/spawns and just use the array to define the order of discharge?

Or will I have to split extensions among multiple spawnCreep calls in the same tick?

@NobodysNightmare

NobodysNightmare Sep 29, 2017

I am not super familiar with the code. Does that mean I could always provide all extensions/spawns and just use the array to define the order of discharge?

Or will I have to split extensions among multiple spawnCreep calls in the same tick?

This comment has been minimized.

@Creosteanu

Creosteanu Sep 29, 2017

Contributor

Yes, you can provide all your extensions/spawns and just use the array order to define the order of discharge.

One additional benefit of doing so is that the system will no longer sort on your behalf, making the call less expensive cpu wise.

@Creosteanu

Creosteanu Sep 29, 2017

Contributor

Yes, you can provide all your extensions/spawns and just use the array order to define the order of discharge.

One additional benefit of doing so is that the system will no longer sort on your behalf, making the call less expensive cpu wise.

@Creosteanu Creosteanu changed the base branch from master to ptr Aug 30, 2017

@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Aug 30, 2017

Contributor

Done. Still need to know what you use to transpile to ES5 to confirm compatibility with args default behavior.

Contributor

Creosteanu commented Aug 30, 2017

Done. Still need to know what you use to transpile to ES5 to confirm compatibility with args default behavior.

@artch

This comment has been minimized.

Show comment
Hide comment
@artch

artch Aug 30, 2017

Contributor

I've tested transpiling, it works fine.

Contributor

artch commented Aug 30, 2017

I've tested transpiling, it works fine.

Show outdated Hide outdated src/utils.js Outdated
Show outdated Hide outdated src/game/structures.js Outdated
@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Sep 18, 2017

Contributor

Ok. The adjustments are tested and appear to be working. I'm leaving the server online on 35.156.32.205. Feel free to do your own testing.

Contributor

Creosteanu commented Sep 18, 2017

Ok. The adjustments are tested and appear to be working. I'm leaving the server online on 35.156.32.205. Feel free to do your own testing.

@ButAds

This comment has been minimized.

Show comment
Hide comment
@ButAds

ButAds Sep 18, 2017

Contributor

I'm testing this now locally. I will let you guys know how/if it works

Contributor

ButAds commented Sep 18, 2017

I'm testing this now locally. I will let you guys know how/if it works

@ButAds

This comment has been minimized.

Show comment
Hide comment
@ButAds

ButAds Sep 20, 2017

Contributor

Tested on a local environment.
The only issue I have with it is that it charges spawns first and extensions later. I'd expect that if I pass energy structures in that it will use the energy in the order I've given them. This is currently not true for when you pass spawns AND extensions.

Contributor

ButAds commented Sep 20, 2017

Tested on a local environment.
The only issue I have with it is that it charges spawns first and extensions later. I'd expect that if I pass energy structures in that it will use the energy in the order I've given them. This is currently not true for when you pass spawns AND extensions.

@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Sep 20, 2017

Contributor

Good point. @artch shall I change it to fully respect the order given?

Contributor

Creosteanu commented Sep 20, 2017

Good point. @artch shall I change it to fully respect the order given?

@artch

This comment has been minimized.

Show comment
Hide comment
@artch

artch Sep 20, 2017

Contributor

It seems reasonable.

Contributor

artch commented Sep 20, 2017

It seems reasonable.

@TheRealMaxion

This comment has been minimized.

Show comment
Hide comment
@TheRealMaxion

TheRealMaxion Sep 20, 2017

I guess renew will still use the default order...
Maybe you can save a custom default order in a way, and also allow specifying per call of createCreep?

TheRealMaxion commented Sep 20, 2017

I guess renew will still use the default order...
Maybe you can save a custom default order in a way, and also allow specifying per call of createCreep?

@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Sep 25, 2017

Contributor

Oki doki.

In order to support proper ordering of energy structures in the new function, I had to fully separate the logic for charge energy.

One healthy side benefit of this approach is that passing energy structures will make spawning significantly more CPU efficient. Though I suppose calls to createCreep are not usually a CPU bottleneck.

Contributor

Creosteanu commented Sep 25, 2017

Oki doki.

In order to support proper ordering of energy structures in the new function, I had to fully separate the logic for charge energy.

One healthy side benefit of this approach is that passing energy structures will make spawning significantly more CPU efficient. Though I suppose calls to createCreep are not usually a CPU bottleneck.

@Creosteanu

This comment has been minimized.

Show comment
Hide comment
@Creosteanu

Creosteanu Sep 25, 2017

Contributor

@TheRealMaxion I would leave any further functionality to another PR. This pull request is starting to gather a lot of functionality which is outside of the original scope. If we keep up this cycle, this PR will never get merged.

Contributor

Creosteanu commented Sep 25, 2017

@TheRealMaxion I would leave any further functionality to another PR. This pull request is starting to gather a lot of functionality which is outside of the original scope. If we keep up this cycle, this PR will never get merged.

@TheRealMaxion

This comment has been minimized.

Show comment
Hide comment
@TheRealMaxion

TheRealMaxion commented Sep 25, 2017

@Creosteanu Fair point.

@artch artch merged commit 560c131 into screeps:ptr Sep 26, 2017

@artch

This comment has been minimized.

Show comment
Hide comment
@artch

artch Sep 26, 2017

Contributor

This PR has been deployed to the PTR, see changelog.

Contributor

artch commented Sep 26, 2017

This PR has been deployed to the PTR, see changelog.

@NobodysNightmare

This comment has been minimized.

Show comment
Hide comment
@NobodysNightmare

NobodysNightmare Sep 29, 2017

I just noticed that spawnCreep (as opposed to createCreep) does not automatically assign a name to created creeps anymore.

I think this is an unneccessary complication for new players, which should not use a deprecated method like createCreep, should they?

Was there a particular reason to leave that behaviour out? The pull request does not at all mention this change, I discovered it by chance after seeing that my spawning code does not work after changing to the new method signature.

In case you want to keep the behaviour as is: The documentation of return values should be updated. Currently it only points at an errorneous creep body, but not at a missing name.

NobodysNightmare commented Sep 29, 2017

I just noticed that spawnCreep (as opposed to createCreep) does not automatically assign a name to created creeps anymore.

I think this is an unneccessary complication for new players, which should not use a deprecated method like createCreep, should they?

Was there a particular reason to leave that behaviour out? The pull request does not at all mention this change, I discovered it by chance after seeing that my spawning code does not work after changing to the new method signature.

In case you want to keep the behaviour as is: The documentation of return values should be updated. Currently it only points at an errorneous creep body, but not at a missing name.

@ButAds

This comment has been minimized.

Show comment
Hide comment
@ButAds

ButAds Sep 29, 2017

Contributor

Hi @NobodysNightmare
Please discuss this in the forums:
https://screeps.com/forum/topic/1947/ptr-changelog-2017-09-26

Kinda regards,
Dissi

Contributor

ButAds commented Sep 29, 2017

Hi @NobodysNightmare
Please discuss this in the forums:
https://screeps.com/forum/topic/1947/ptr-changelog-2017-09-26

Kinda regards,
Dissi

@NobodysNightmare

This comment has been minimized.

Show comment
Hide comment
@NobodysNightmare

NobodysNightmare Sep 29, 2017

Hey @ButAds,

sure I can do this there too... just note that @artch guided me here in the first place oO

edit: Went for the non-PTR changelog

NobodysNightmare commented Sep 29, 2017

Hey @ButAds,

sure I can do this there too... just note that @artch guided me here in the first place oO

edit: Went for the non-PTR changelog

@ButAds

This comment has been minimized.

Show comment
Hide comment
@ButAds

ButAds Sep 29, 2017

Contributor

Ah, I did not know that! It's kinda hard to keep track of everything in different places. That's why I originally asked you to move the conversation to the forums. I will reply there. Sorry for the confusion

Contributor

ButAds commented Sep 29, 2017

Ah, I did not know that! It's kinda hard to keep track of everything in different places. That's why I originally asked you to move the conversation to the forums. I will reply there. Sorry for the confusion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment