Power Creeps and Effects #258

shanemadden · 2019-10-23T00:57:34Z

Added power creeps and the necessary game and power spawn object interfaces to work with them, as well as effects on room objects which are used by both power creeps and strongholds.

Effects can have types representing powers or 'natural effects' (which only strongholds use currently) - this is set up as a nested enum to keep PowerType distinct from EffectType, though these could also just be a single enum with the relevant integer representations of both power and natural effects if that's overcomplicating things.

Implementations on creeps which are identical to those on power creeps have been moved to a SharedCreepProperties trait which both creeps and power creeps use - I'm not a huge fan of this naming if there's other ideas.

Several attributes on power creeps not spawned in the world are undefined in the same way as the issue reported in #246, causing a number of potential panics. This switches just the directly-impacted functions to return a Result, which might be a start toward #3 - currently just bubbling up serde's ConversionErrors which seems potentially not ideal? If this ends up being the final implementation it's probably a good time/release to make similar result changes in other places.

Resolves #158

…uire it

…s in comments and errors

daboross · 2019-10-24T01:45:32Z

src/objects.rs

@@ -120,12 +122,12 @@ reference_wrappers! {
 ///
 /// This can be freely implemented for anything with a way to get a position.
 pub trait HasPosition {
-    fn pos(&self) -> Position;
+    fn pos(&self) -> Result<Position, ConversionError>;


While this technically solves the problem, I'd argue changing the traits like this doesn't really take advantage of the type system we have.

Specifically, changing the traits like this requires everyone to deal with usually-non-existent error cases dealing with every other non-power-creep object.

In addition, this makes methods like .get_range_to panic when called with a power creep, and there's no indication in the code that something dangerous has happened. If users had to manually unwrap a PowerCreep result in order to do this, I think that could make things clearer.

The idea I'm coming from here is that ideally, things which behave differently or could fail should be dealt with explicitly by consumers. Users of the crate should be forced to deal with error cases, and shouldn't be able to write code which has a chance of failing at runtime without knowing (without doing an .unwrap() or .expect() somewhere in their own code).

What would you think of just not implementing HasPosition and other traits for PowerCreep and instead requiring first converting to an "InstantiatedPowerCreep" or similar before being able to call relevant methods? Or if not that, what are your thoughts on this problem?

Edit: edited shortly after posting to refine my thoughts better.

Agreed, this kind of change to these methods makes them a lot more awkward to use - my initial attempt at implementation used an enum to wrap an alive-or-dead-power-creep, but that ended up being a mess - this sounds better ;)

The comparison I'm thinking of is that we'd have something akin to a std::rc::Weak upgrade into a std::rc::Rc - taking the maybe-alive-power-creep object reference and upgrading it to get a we-know-it's-alive object with room object properties, HasId, Attackable etc.

Because this is only applicable to the results of accessing game::power_creeps (and game::creeps in the #246 case) case and not for objects found via find()/look(), my thinking is to have PowerCreep as the 'fully alive power creep' representation (which would be the find/look result, intuitively) and to add PowerCreepInfo or GamePowerCreep or somesuch which would be the version of the reference returned by game::power_creeps::* that can be upgrade()-ed to a full PowerCreep if it's spawned on the current shard.

I could use some guidance on how to get this implemented - I can't quite get my head around how to get that 'upgrade()' function set up to get the underlying js ref moved from PowerCreepInfo/ShrodingersPowerCreep to PowerCreep during the upgrade(), to get it converted to Option<PowerCreep>.

Worked it out and went with AccountPowerCreep, should be a much less breaking changeset now :)

Awesome!

The comparison I'm thinking of is that we'd have something akin to a std::rc::Weak upgrade into a std::rc::Rc - taking the maybe-alive-power-creep object reference and upgrading it to get a we-know-it's-alive object with room object properties, HasId, Attackable etc.

This sounds like a sane comparison to me. The new scheme looks good!

ShrodingersPowerCreep

🤣

I think I like it better than AccountPowerCreep 😛

… that can be converted to a full PowerCreep if spawned

shanemadden · 2019-10-26T14:40:01Z

Ok, last implementation question - should I add a wrapper around game::creeps::* returns to account for the potentially missing id in #246 in the same way AccountPowerCreep does for power creeps or leave fixing that for another time?

ASalvail

This is another one that is looking quite good!

src/constants/types.rs

ASalvail · 2019-10-27T18:41:25Z

Ok, last implementation question - should I add a wrapper around game::creeps::* returns to account for the potentially missing id in #246 in the same way AccountPowerCreep does for power creeps or leave fixing that for another time?

This one's big enough already. I suggest keeping it separate. PRs aren't expensive!

…, update tower repair to match creep repair, fmt again with nightly to fix comments

shanemadden · 2019-12-22T17:28:18Z

@ASalvail @daboross Have a couple more minor changes on here that could use a quick eyeball before they get merged - thanks!

ASalvail

Changes look good. Thanks!

ASalvail · 2019-12-22T22:04:49Z

src/constants/numbers.rs

@@ -276,8 +276,7 @@ pub const INVADERS_ENERGY_GOAL: u32 = 100000;

 pub const SYSTEM_USERNAME: &str = "Screeps";

-pub const SIGN_PLANNED_AREA: &str =
-    "A new Novice or Respawn Area is being planned somewhere \
+pub const SIGN_PLANNED_AREA: &str = "A new Novice or Respawn Area is being planned somewhere \


rustfmt changed it's default value?

The joys of the nightly branch apparently!

ASalvail · 2019-12-22T22:06:53Z

src/objects/impls/structure_tower.rs

        js_unwrap! { @{self.as_ref()}.attack( @{target.as_ref()} ) }
    }

-    pub fn heal(&self, target: &Creep) -> ReturnCode {
+    pub fn heal<T>(&self, target: &T) -> ReturnCode


Can you heal enemy creeps?

Yup, sure can! You usually wouldn't want to, but there is a useful uncommon case in healing allies' creeps (though my intent for this change was just to make it consistent with the args for the heal function on Creep)

shanemadden added 4 commits October 22, 2019 18:26

Power Creeps, Effects, and Result<T, E> switch for functions with req…

91826a9

…uire it

Fix tests for new result returns

92d6d4e

Correct OperateObserve name to OperateObserver, correct constant name…

0a3f184

…s in comments and errors

Add a bit of documentation, update some unwraps to clearer expects, fmt

db826e8

daboross reviewed Oct 24, 2019

View reviewed changes

shanemadden added 3 commits October 25, 2019 13:17

Change from adding error returns to using a limited AccountPowerCreep…

1ba1b75

… that can be converted to a full PowerCreep if spawned

cargo fmt

f9c5538

changelog and fix incomplete comment

ea5701e

ASalvail approved these changes Oct 27, 2019

View reviewed changes

src/constants/types.rs Outdated Show resolved Hide resolved

shanemadden added 5 commits October 28, 2019 07:44

Roll back unneeded change to PowerType size

5d7c7ea

Update tower functions and creep heal to allow targeting power creeps…

522fdb4

…, update tower repair to match creep repair, fmt again with nightly to fix comments

Merge branch 'master' into power_and_effects

d00483a

Switch to taking &str instead of an owned String for power creep name

9ff4378

cargo fmt

bfe1034

ASalvail approved these changes Dec 22, 2019

View reviewed changes

shanemadden merged commit 751af23 into rustyscreeps:master Dec 27, 2019

shanemadden deleted the power_and_effects branch December 27, 2019 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Power Creeps and Effects #258

Power Creeps and Effects #258

shanemadden commented Oct 23, 2019 •

edited

Loading

daboross Oct 24, 2019 •

edited

Loading

shanemadden Oct 24, 2019 •

edited

Loading

shanemadden Oct 25, 2019

daboross Oct 25, 2019

ASalvail Oct 27, 2019

shanemadden commented Oct 26, 2019

ASalvail left a comment

ASalvail commented Oct 27, 2019

shanemadden commented Dec 22, 2019

ASalvail left a comment

ASalvail Dec 22, 2019

shanemadden Dec 26, 2019

ASalvail Dec 22, 2019

shanemadden Dec 26, 2019

Power Creeps and Effects #258

Power Creeps and Effects #258

Conversation

shanemadden commented Oct 23, 2019 • edited Loading

daboross Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

shanemadden Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

shanemadden Oct 25, 2019

Choose a reason for hiding this comment

daboross Oct 25, 2019

Choose a reason for hiding this comment

ASalvail Oct 27, 2019

Choose a reason for hiding this comment

shanemadden commented Oct 26, 2019

ASalvail left a comment

Choose a reason for hiding this comment

ASalvail commented Oct 27, 2019

shanemadden commented Dec 22, 2019

ASalvail left a comment

Choose a reason for hiding this comment

ASalvail Dec 22, 2019

Choose a reason for hiding this comment

shanemadden Dec 26, 2019

Choose a reason for hiding this comment

ASalvail Dec 22, 2019

Choose a reason for hiding this comment

shanemadden Dec 26, 2019

Choose a reason for hiding this comment

shanemadden commented Oct 23, 2019 •

edited

Loading

daboross Oct 24, 2019 •

edited

Loading

shanemadden Oct 24, 2019 •

edited

Loading