Implement various methods. #38

ASalvail · 2018-09-10T04:57:59Z

Best reviewed per commit.

ASalvail · 2018-09-16T07:34:45Z

Addresses a lot of #30 points.
What's left is everything that handles paths and a way to represent whatever look methods return.

Those will be separate PRs however, so this one should stand on its own.

daboross

First pass on this!

Overall I'm super glad to have these implemented, and the majority of them look all good. There are a few ones which I think could be changed, and those are noted in the review.

Hope it's alright that I've just commented on the final changeset rather than the individual commits for each part.

Thanks!

daboross · 2018-09-17T07:17:34Z

screeps-game-api/src/constants.rs

@@ -775,10 +821,59 @@ impl ResourceType {
            CatalyzedGhodiumAlkalide => 90,
        }
    }
+
+    pub fn to_string(&self) -> String {


I'd prefer to use the JS methods present at https://github.com/daboross/screeps-in-rust-via-wasm/blob/master/screeps-game-api/javascript/utils.js#L97 to do variant -> string conversion instead of converting to a string inside Rust wherever possible.

It's mostly for efficiency as translating strings across the rust/js boundary is much more expensive than simple integers.

Sorry about not documenting the use of /rational for those methods better!

If we do want this method anyways, I think it could be changed to return &'static str rather than String? All of the strings here are static so it might be a bit wasteful to allocate a new string per use. If we do that renaming it to as_str like mentioned below might be a good idea.

Argh, I forgot about those!
I don't think having the string instead of the variant is useful. It was only meant to convert to a string before passing the hand to JS.

How about a wrapper around those JS functions that would implement From<ResourceType> for Value::String and TryFrom<Value::String> for ResourceType using those js methods instead of the usual conversion provided from stdweb?

The problem with TryFrom<Value::String> is that at that point the string has already been loaded into Rust, so it's equivalent to doing a method like this.

Value::String just wraps a rust String, not a pointer to a JS string. If we construct it or consume it at all we're already paying the cost of transfering the string between Rust and JS, which is what I want to avoid.

daboross · 2018-09-17T07:18:35Z

screeps-game-api/src/game.rs

@@ -60,7 +60,7 @@ pub mod cpu {
    ///
    /// [http://docs.screeps.com/api/#Game.getHeapStatistics]: http://docs.screeps.com/api/#Game.getHeapStatistics
    ///
-    /// Returns object with all 0 values if heap statistics are not available.
+    /// Returns object with all 0 values if heap statistics are not availe.


Accidentally included?

Yup, thanks

daboross · 2018-09-17T07:19:03Z

screeps-game-api/src/game.rs

@@ -170,12 +172,185 @@ pub mod map {

    /// See [http://docs.screeps.com/api/#Game.map.isRoomAvailable]
    ///
-    /// [http://docs.screeps.com/api/#Game.map.isRoomAvailable]: http://docs.screeps.com/api/#Game.map.isRoomAvailable
+    /// [http://docs.screeps.com/api/#Game.map.isRoomAvaile]: http://docs.screeps.com/api/#Game.map.isRoomAvaile


Accidentally included?

🤔 now those I think I corrected at some point, they might have reverted.... somehow 😕

daboross · 2018-09-17T07:28:54Z

screeps-game-api/src/objects/impls/room.rs

+    /// Unimplemented
+    #[allow(unused_variables)]
+    pub fn look_at(&self, x: u32, y: u32) -> ! {
+        unimplemented!()


See above comment on unimplemented!().

daboross · 2018-09-17T07:28:58Z

screeps-game-api/src/objects/impls/room.rs

+    /// Unimplemented
+    #[allow(unused_variables)]
+    pub fn look_at_area(&self, top: u32, left: u32, bottom: u32, right: u32) -> ! {
+        unimplemented!()


See above comment on unimplemented!().

daboross · 2018-09-17T08:02:36Z

screeps-game-api/src/constants.rs

+impl TryFrom<String> for ResourceType {
+    type Error = String;
+
+    fn try_from(s: String) -> Result<Self, Self::Error> {


See comment on to_string: I'd prefer to use integers to translate the various types across the rust<->js boundary whenever possible.

Not against having this implemented, but I think it'd be good to at least mention the str->int translations available on the JS side in a doc-comment here, and to warn about not using this if not necessary.

daboross · 2018-09-17T08:04:58Z

screeps-game-api/src/game.rs

+
+    #[allow(unused_variables)]
+    pub fn find_route(from_room: Room, to_room: Room, route_callback: Option<impl Fn(&str, &str) -> u32>) -> !{
+        unimplemented!()


I'd rather leave these methods out for the sake of having compile-time errors rather than runtime ones when trying to use them.

We could leave it in here commented out for the sake of keeping the signature, though.

Thoughts?

I like having them in the code because of the documentation generated (granted, this one doesn't have a doc).
I believed that using a function with the never return type wouldn't compile. After testing... it's annoying and confusing, so commenting it out seems like the best option.

We could also have an unimplemented-functions feature which we enable when building docs but disable otherwise.

Or we could also change the function to accept a never/void input parameter '_unimplemented'! Never type in the return position can actually be called, it just guarantees the function will only ever panic or loop forever. If we include something like enum UnimplementedParameterVoid {} and then take _unimplemented: UnimplementedParameterVoid that would prevent anyone from calling it since no one would be able to construct that value.

daboross · 2018-09-17T08:07:21Z

screeps-game-api/src/game.rs

+    }
+
+    impl OrderType {
+        fn as_string(&self) -> String {


Since we are converting to a different type, to_string would be more appropriate than as_string.

See https://rust-lang-nursery.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv.

In addition, returning &'static str could be better here. Though if we do that, as_str might be a more appropriate name after all - it would indeed be free.

Would you rather we apply the same principle as for the resources and pass an integer instead? Again, this is solely used to call into JS, otherwise, the type is more than explicit enough.

If you think it's sane, sure!

I'm not 100% sure this optimization is the best idea, but then again it does seem worth it without much cost (just writing JS code to do the string->variant conversion instead of Rust code). I originally added it for resource types just because I imagined I would use those a lot. If it wouldn't be too much trouble an __order_type_num_to_str and __order_type_str_to_num pair seems like it'd be beneficial to me.

Sorry, I think that was a bit rambly/not clear.

I'd go ahead with this and apply the same principles. I'm not completely sure that it's the best idea, but I like it anyways, and I'm confident it will make the conversions faster.

daboross · 2018-09-17T08:09:00Z

screeps-game-api/src/game.rs

+        }
+    }
+
+    pub fn deal(order_id: &str, amount: u32, target_room: Room) -> ReturnCode {


Could this take &Room instead?

I'd almost want to have it taking a room name &str so that someone could execute deals without grabbing a reference to their room, but I don't feel too strongly about that.

I do, it was a mistake on my part. Fixed!

daboross · 2018-09-17T08:09:36Z

screeps-game-api/src/objects/impls/creep.rs

@@ -38,10 +45,19 @@ impl Creep {
        js_unwrap!(@{self.as_ref()}.moveTo(@{x}, @{y}))
    }

+    #[allow(unused_variables)]
+    pub fn move_by_path(path: String) -> ! {
+        unimplemented!()


See above comment on unimplemented!().

ASalvail · 2018-09-18T03:56:14Z

Thanks a lot for taking the time to read through these.

I addressed most small things. I'll wait for your feedback on the points raised above before finishing the corrections.

daboross · 2018-09-18T05:42:45Z

I'm glad to do it- thank you for doing this huge implementation pr!

I've replied to the individual threads.

js_unwrap! is not updated as it's not touched by this commit and will be fixed by #38.

ASalvail · 2018-09-20T00:51:27Z

There you go, that should address all of your concerns 😃

ASalvail · 2018-09-20T01:58:51Z

Addresses most of #23

daboross · 2018-09-20T20:57:41Z

LGTM! 👍

I think travis is complaining because of changes on the master branch so fixing that after merging seems sanest.

Opened #52 for one lingering issue that I think could easily be dealt with separately.

daboross · 2018-09-21T03:30:38Z

screeps-game-api/src/game.rs

+    }
+
+    // impl OrderType {
+    //     fn as_string(&self) -> String {


If you do want to keep this method in, I don't think it would hurt.

If we want to add in a typed OrderType to the transactions, the serde translation code would have to use strings anyways since stdweb turns it into JSON before loading it in.

js_unwrap! is not updated as it's not touched by this commit and will be fixed by #38.

daboross added the A-screeps-game-api label Sep 15, 2018

ASalvail added 22 commits September 15, 2018 17:26

Implement various methods.

d8f45c6

Market complex methods

a76d1ac

Flaggy Flag!

4664e3e

Mineral + ResourceType conversion to string

3069b27

Nukes to ruin your day

34964e2

Add some methods to Room

79ca6a7

Add a method for RoomPosition

f4d650f

Add a CanStoreEnergy trait

7d284b5

Add a HasCooldown trait

b3d642e

Add method to KeeperLair in mod.rs

b32bfa9

Add StructureLab module and conversion from string to Resource type

bfb79c6

Add structure_link module and implement method

4308afa

Add structure_nuker mod + implement methods

2d0735d

Add structure_observer mod

3869700

Add trait CanDecay

fc74433

Add PowerBank accessor to mod.rs

5661dbc

Add structure_power_spawn module

c8e84ed

Add structure_portal module

f350548

Add structure_rampart module

8c36aa6

Add structure_terminal module

756a96f

Add structure_tower module

ca8a521

Import all changes into impls module

0dde821

ASalvail changed the title ~~[WIP] Implement various methods.~~ Implement various methods. Sep 16, 2018

ASalvail added 2 commits September 16, 2018 03:23

Add new trait to prelude

d7514e2

Moved hits and hitsMax to the Attackable trait

82f81a2

daboross requested changes Sep 17, 2018

View reviewed changes

Addressed small changes from @daboross review.

75418e3

daboross added a commit that referenced this pull request Sep 18, 2018

Include semicolon in js_unwrap_ref!

4433ef6

js_unwrap! is not updated as it's not touched by this commit and will be fixed by #38.

daboross added a commit that referenced this pull request Sep 18, 2018

Include semicolon in js_unwrap_ref!

55c86c4

js_unwrap! is not updated as it's not touched by this commit and will be fixed by #38.

ASalvail added 3 commits September 19, 2018 20:30

Transfers string conversion to JS side for ResourceType

4420339

Converts OrderType to a number repr for quick transfer

402c5fd

Removed Attackable trait from StructureController

d5c1a05

daboross approved these changes Sep 20, 2018

View reviewed changes

daboross mentioned this pull request Sep 20, 2018

order_type should be typed as OrderType rather than String in market APIs #52

Closed

daboross merged commit e3dc452 into rustyscreeps:master Sep 20, 2018

daboross reviewed Sep 21, 2018

View reviewed changes

daboross added a commit that referenced this pull request Sep 25, 2018

Include semicolon in js_unwrap_ref!

e48d00f

js_unwrap! is not updated as it's not touched by this commit and will be fixed by #38.

daboross added a commit that referenced this pull request Sep 25, 2018

Include semicolon in js_unwrap_ref!

31d772e

js_unwrap! is not updated as it's not touched by this commit and will be fixed by #38.

ASalvail mentioned this pull request Oct 15, 2018

HasId trait + other Has* traits for common properties #23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement various methods. #38

Implement various methods. #38

ASalvail commented Sep 10, 2018 •

edited

Loading

ASalvail commented Sep 16, 2018

daboross left a comment

daboross Sep 17, 2018

ASalvail Sep 18, 2018

daboross Sep 18, 2018

daboross Sep 17, 2018

ASalvail Sep 18, 2018

daboross Sep 17, 2018

ASalvail Sep 18, 2018

daboross Sep 17, 2018

daboross Sep 17, 2018

daboross Sep 17, 2018

daboross Sep 17, 2018

ASalvail Sep 18, 2018

daboross Sep 18, 2018

daboross Sep 17, 2018

ASalvail Sep 18, 2018

daboross Sep 18, 2018

daboross Sep 18, 2018

daboross Sep 17, 2018

ASalvail Sep 18, 2018

daboross Sep 17, 2018

ASalvail commented Sep 18, 2018

daboross commented Sep 18, 2018

ASalvail commented Sep 20, 2018

ASalvail commented Sep 20, 2018

daboross commented Sep 20, 2018

daboross Sep 21, 2018

Implement various methods. #38

Implement various methods. #38

Conversation

ASalvail commented Sep 10, 2018 • edited Loading

ASalvail commented Sep 16, 2018

daboross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ASalvail commented Sep 18, 2018

daboross commented Sep 18, 2018

ASalvail commented Sep 20, 2018

ASalvail commented Sep 20, 2018

daboross commented Sep 20, 2018

Choose a reason for hiding this comment

ASalvail commented Sep 10, 2018 •

edited

Loading