4. Make Creeps and PowerCreeps inherit from a "Movable" class

Make Creeps and PowerCreeps inherit from a "Movable" class


  • @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, but master 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 with var x = Creep.prototype.move.
    • grabbing Creep.moveTo with var x = Creep.prototype.moveTo.
    • grabbing Creep.moveByPath with var 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 expecting somePowerCreep.moveTo() to use the new creep moveTo function
    • overwriting Creep.prototype.moveByPath = x; and then expecting somePowerCreep.moveByPath() to use the new Creep moveByPath 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.

    1 Reply

  • @daboross yeah, at best, it'd be a minor breaking change... and seeing as modifying Creep.prototype.move etc affecting PowerCreep 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 affecting PowerCreep.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 than Movable.


  • Here is a behavior change. Object.getOwnPropertyNames(Creep.prototype) would no longer return move or moveTo.

    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 affecting PowerCreep.move.


  • Ok everyone, @ags131 very graciously set up a pServ at prtest.screepspl.us as a test with my fork/branch of engine with a BaseCreep 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

    1 Reply

  • @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 the FIND_POWER_CREEPS and FIND_CREEPS calls. For completeness add a FIND_MY_MOVEABLES as well.


  • @SemperRabbit It looks like this is branched from main. It should branch from PTR, otherwise this will force all the new Store to code to rebased (merge-hell).


  • its ok @deft-code, ty for the suggestions, but yeah, i guess its not happening ever. devs put their foot down, testing be damned.

    ☹

  • @artch, I have to say that in light of the recent update I am now quite confused by the decision not to do anything about this.

    I would urge you to rethink your previous assertion that this would be a major breaking change. The community has put in significant effort to investigate and show that this change would actually have very minimal impact.

    The strongholds update did actually contain several major breaking changes, so clearly you are willing to make changes if the benefits are clear. I think they are clear in this case. The current situation results in strange and confusing bugs and there is no advantage to keeping it this way other than some notion of backwards compatibility, which is pretty moot since this has been shown to be almost entirely backwards compatible. Additionally, it is exactly those players that would be affected who are asking for this to be changed.

  • Dev Team

    We will revisit this topic when we come back to Power Creeps (Operator) balance in the nearest future.

    👍