Make Creeps and PowerCreeps inherit from a "Movable" class
-
My main issue is the cross-prototype calls in the power creep like this:
PowerCreep.prototype.moveTo = register.wrapFn(function(firstArg, secondArg, opts) { if(!this.room) { return C.ERR_BUSY; } return globals.Creep.prototype.moveTo.call(this, firstArg, secondArg, opts); });
It looks innocuous but there's a problem here; It goes against expectations (and general OOP).
Creep.prototype.moveTo
is expected to only be called on creeps. Therefore any overrides, any internal behavior of moveTo, expectsthis
to be an instance of aCreep
, or a class derived from one. A PowerCreep is neither, it's an entirely separate class abusing the scope of a Creep method. There are several methods and properties like this, that could be written once and reused, but the only common base isRoomObject
which for obvious reasons is too abstract for this.I understand why a PowerCreep can't simply be a child class of Creep, too many differences. But If you find yourself writing calls like this it's time to consider writing a shared based class.
This would allow for things like
hits
,carry
,moveTo
,my
,owner
, etc and any other methods or properties identical between the two to be written once and reused across as many types of actors as you want.I'm a strong proponent for having a shared base class. Due to the way prototype inheritance works, adding such a prototype to the engine would not cause any breaking changes to existing player code.
-
I support a base class as well: the typescript types use
AnyCreep
as a base to proxy this for e.g. methods likeTower.attack
which should be able to attackCreep | Structure | PowerCreep
(note: the current documentation does not include PowerCreep as a valid target type)
-
I can understand why they didn't do this - literally EVERY player's code is heavily reliant on creeps being a certain way, and now a lot of code also depends on power creeps being a certain way. If you're the kind of person who messes with prototypes, first why? secondly, changing this to a base class will likely break your code - it's not something that can be easily deprecated. Also, there's inactive players you need to think about - if you leave the game for two months and come back to find everything has been broken for a month, I'd be pretty pissed and never come back.
On the other hand, I totally agree with what you're saying: creeps and power creeps should inherit from a common base class, not just for simplicity, but as a general rule of programming too. I'd definitely prefer my code to simply "work" with little to no modification once I reach the power creep stage.
The real problem is that this isn't a cut-and-dry issue. There are nuances that need to be considered at all levels. I can totally understand the player base wanting something, and the developers simply being unable to deliver. They're really stuck between a rock and a hard place here.
But that's my view as a gamedev myself, so I may be biased.
-
@ratstail91 That doesn't really explain why they didn't do this to begin with though. "Legacy Code" is a poor excuse for bad design when you get to start from scratch (and they did get to start from scratch).
This wouldn't be the first time they made a breaking change- it wouldn't even be the first time they made a breaking change without pushing it to PTR first. Yeah, it's possible that inactive players will suffer for that, but realistically if you're not paying attention to the PTR and whatnot, you don't really have any right to get upset. You're actively not participating in the game- does it make a difference if you get wiped because one of your neighbors started eyeballing your territory vs because you didn't pay attention to patch notes?
User error is not a valid excuse for bad design either.
Personally, I disagree- this is absolutely a cut and dried issue. I'm all for recognizing the nuance in complex situations, but the truth is that the longer this is left unchanged the more problematic is becomes. Precedent exists for pushing breaking changes.
Besides, note- Both Warinternal and Semp are players that use prototypes heavily. @WarInternal I know for certain uses, for lack of a better phrase, a "Prototype-based architecture". The idea that these changes shouldn't be made for the sake of players who rely heavily on prototypes is completely swatted down by the fact that those very players are the ones asking for these changes.
-
One quirk of power creeps is that if you change a method on the
Creep
prototype which is also used onPowerCreep
, then power creeps will use that method as well. I'm not sure whether this should be kept or removed. Personally, I think the proper behavior would be to remove this quirk, but I don't know whether anyone depends on it (intentionally or accidentally).
-
@knightshade OK, fair enough. Like I said, I'm totally on the side of refactoring the creeps, and I've only been playing for two weeks, so I don't know about any precedents, but it was just my view as a gamedev, and how I would run the game.
-
Bumping because the devs never responded, although they responded to other stuff. Giving them the benefit of the doubt that they missed it...
-
I recently started with power creeps and I completely agree that there should be a shared base class. The fact that power creeps call methods on
Creep
caused me lots of issues.
-
Introducing a new base class behind Creep class is a major breaking change for userspace code. Not just a breaking change, but a major breaking change, it will affect too many things (not in the engine, but in players' code). We will only consider such changes in some opt-in backward-incompatible versions of the game API, like 2.0 or something, but not in the current version.
-
With Iso-VM being a thing, this should be a viable option, no?
EDIT: Actually, would this even be a significantly breaking change? Only players who had gone into the power creep move prototype would be having problems with the change breaking, and those players would explicitly be the ones trying to fix this problem.
Not only that, was removing preboosting not a breaking change? And that was a hell of a lot more widespread than people mucking about in the power creep movement prototype.
Come to think of it, you could just as easily just point both the power creep and regular creep
move
commands to the prototype they're inheriting from and mark them as depreciated. That both wouldn't break anything and allow would allow people to get their code fixed.
-
@artch Due to how prototype inheritance works I don't believe this would break anything.
If we introduce a LivingEntity (choose any name) class between RoomObject and PowerCreep/Creep, the empty class by itself will change exactly nothing in the behavior. Existing properties and methods will still be inherited.
If we then move the method
move
to it, both classes would inherit the method as-is. The move methods on PowerCreep could be removed entirely, as instead of a hacky cross-prototype call, they'd inherit them from the LivingEntity class. And If you ever wanted to introduce a third type of movable actor you'd already have a decent base class to work with.As for the common monkey-patching that we do that is 1) never been officially supported I thought and 2) surprise, still not broken. If you monkey-patch move on the Creep method, it will still inherit from the super class, only now it's not breaking PC because they have their own copy of move, from the LivingEntity class, and we have the option of patching that if we want to affect both.
If anybody can think of something it would break, I'm all ears.
Edit:
As for the
if (!this.room)
check in PC move, a prototype call can still be made but it needs to stay within it's own chain. If PC inherited from LivingEntity, something like:PowerCreep.prototype.moveTo = register.wrapFn(function(firstArg, secondArg, opts) { if(!this.room) { return C.ERR_BUSY; } return globals.LivingEntity.prototype.moveTo.call(this, firstArg, secondArg, opts); });
would be acceptable, if LivingEntity were the parent class to PowerCreep.
-
@artch, @o4kapuk, check out this fork of the engine. I have successfully tested this with creeps with both my and an Overmind codebase for approx 160k ticks each. neither of them broke at all. I had to build it with the
refact-remove-ivm
branch of the driver repo, butmaster
for everything else... @GimmeCookies said he would test PowerCreep code on it later today. so far, we have not found any breaking changes in the slightest. I would love for others to test it too (ideally including the devs), to see if there is anything to break.
-
Honestly, this idea makes a lot of sense.
@SemperRabbit has a working branch of the code, and I can't see a single thing that would break if done properly (since seamless inheritance like this is literally the point of objects)
@artch can you name a single thing that would actually break with this change?
-
I can think of several things which wouldn't break with this?
- grabbing
Creep.move
withvar x = Creep.prototype.move
. - grabbing
Creep.moveTo
withvar x = Creep.prototype.moveTo
. - grabbing
Creep.moveByPath
withvar x = Creep.prototype.moveByPath
. - calling
c.moveTo()
with any arguments - replacing the Creep move prototype with
Creep.prototype.move = x;
and having creep movement use the new function - replacing the Creep moveByPath prototype with
Creep.prototype.moveByPath = x;
and having creep movement use the new function - replacing the Creep moveTo prototype with
Creep.prototype.moveTo = x;
and having creep movement use the new function
The only thing which would break, as far as I can tell, are:
- overwriting
Creep.prototype.moveTo = x;
and then expectingsomePowerCreep.moveTo()
to use the new creep moveTo function - overwriting
Creep.prototype.moveByPath = x;
and then expectingsomePowerCreep.moveByPath()
to use the new CreepmoveByPath
function
Both of these things are breaking only to people who've written PowerCreep code in the time since they've been out, and both have the easily fixable solution of overwriting
Moveable.moveTo
/Moveable.moveByPath
. There's no way this can affect idle users, even if they extensively use the prototype system.
- grabbing
-
@daboross yeah, at best, it'd be a minor breaking change... and seeing as modifying
Creep.prototype.move
etc affectingPowerCreep
stuff is not documented in the API, no one has any right to get butt hurt about it changing. it's like if someone were to rely on the exact order of a.find()
. if that changes and breaks their code, who's fault is that? Never trust anything that's not in the API...
-
I would argue that
Creep.move
affectingPowerCreep.move
is itself a bug. This surprised me when I encountered it and I fully expected it to be fixed at some point. Anyone relying on this behaviour is relying on a bug and should expect it to be fixed. For what it's worth, fixing this will break my code, but I still want it to be fixed.All you need to do is publish an announcement far enough in advance that this is the plan and anyone relying on it can prepare for it by doing this:
PowerCreep.prototype.move = Creep.prototype.move; PowerCreep.prototype.moveTo = Creep.prototype.moveTo; PowerCreep.prototype.moveByPath = Creep.prototype.moveByPath; PowerCreep.prototype.transfer = Creep.prototype.transfer; PowerCreep.prototype.withdraw = Creep.prototype.withdraw; PowerCreep.prototype.drop = Creep.prototype.drop; PowerCreep.prototype.pickup = Creep.prototype.pickup; PowerCreep.prototype.say = Creep.prototype.say;
For anyone who does this or isn't relying on the buggy behaviour there are no breaking changes!
Note: I would strongly suggest that all of the offending methods above should be fixed, not just the move ones, so the class should probably be called
BaseCreep
or something rather thanMovable
.
-
Here is a behavior change.
Object.getOwnPropertyNames(Creep.prototype)
would no longer returnmove
ormoveTo
.I assumed it would break more stuff. I was surprised that
Creep.prototype.move
still worked the same.I really think this is a good idea. And it fixes the bug of changes to
Creep.move
affectingPowerCreep.move
.
-
Ok everyone, @ags131 very graciously set up a pServ at prtest.screepspl.us as a test with my fork/branch of
engine
with aBaseCreep
class. He and @WarInternal already helped me identify/fix 1 error in it, and it is running pretty smoothly right now. Please spawn in and post results if you can@artch, idk if you have any code, but you and @o4kapuk are more than welcome to spawn in too. I'd love to get feedback on what the devs think. btw, figuring out the
data()
call was more tricky than I thought (see the commits from today lol)Again, everyone can take a look at my branch here: https://github.com/semperrabbit/screeps_engine/tree/BaseCreep
-
@semperrabbit Thank you for taking the initiative on this Semp. This is huge. I'd like to recommend adding a FIND_MOVEABLES or something similar so there can be a unified lookup, and probably others I'm not thinking of.
How is pull handled here?
-
@SemperRabbit No pull request so I'll leave code review comments here.
Use braces on all multi line if statements. That seems to be the style in the rest of the repo.
Consider breaking this up into two PRs. The first one creates the new base-creep type in the existing creep file renaming classes but otherwise not moving code around. The second one moves it all to the new file. This would be far easier to review as the unchanged functions would be nearly diff free. The second one would have tons of diffs but very little new code. The easier the PR is to review the more likely the devs will accept it.
We're still having memory problems with high room counts. I suspect the indexes are to blame.
FIND_MOVEABLES
is a fine idea but I would discourage a new index just concat theFIND_POWER_CREEPS
andFIND_CREEPS
calls. For completeness add aFIND_MY_MOVEABLES
as well.