Implements Room.findPath() #53

ASalvail · 2018-09-21T01:25:27Z

I'm very unsure of this one. For some obscure reasons (that might end up in our favor?!), Room.findPath() uses a mutable reference to the cost matrix to make its changes. I adapted the code in that direction, but kept to the scoped_lts approach.

However, since a CostMatrix is received as input, it doesn't need lifetime annotations anymore. The thing is, I don't know enough about the intricacies of rust <-> wasm interractions to actually make a judgement call about whether this is still necessary.

I'm all ear to learn about it however 😄

ASalvail · 2018-09-21T02:50:00Z

Correct me if I'm wrong, but the whole shenanigans with the closure that was needed for the pathfinder::search was due to the CostMatrix being generated from the callback, thus making it's lifetime hard to infer by the rust compiler.

In that case, implementing a callback for map.findRoute should not need any of that and simply use the callback functionalities provided by stdweb. Is that correct?

daboross · 2018-09-21T03:53:04Z

Unfortunately the lifetime shenanigans are related to having a closure at all that we pass into JavaScript which is not 'static. I think we'll need it any time we want to have full capability in callbacks.

daboross

I think the lack of lifetime annotations is correct- JS owns a CostMatrix, we have a reference to it. JS's CostMatrix will last as long as we want since we have a reference so it doesn't need a lifetime.

Here are a few other comments just about the current state of this.

daboross · 2018-09-21T03:42:17Z

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

+    where
+        O: HasPosition,
+        T: HasPosition,
+        F: Fn(String, CostMatrix) -> (),


The function should be able to return a CostMatrix in order to overwrite the one provided by the room- this probably wants to be -> Option<CostMatrix>.

Honnestly, my first thought was not to support replacing the cost matrix. It is both inneficient and a pain to implement.

screeps-game-api/src/constants.rs

daboross · 2018-09-21T03:48:07Z

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

 };
+use HasPosition;


If you feel like cleaning up the use style, this should be in the objects:: group. It's probably not correct anywhere else anyways though until #49 is merged.

daboross · 2018-09-21T03:51:28Z

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

+        from_pos: &O, 
+        to_pos: &T,
+        opts: FindOptions<F>,
+    ) -> Vec<Step> 


This only handles non-serialized paths.

Idea:

We could make FindOptions generic over "F: FindResultType", a trait like:

trait FindResultType { type Result; fn deserialize(v: Value) -> Result; fn serialized() -> bool; }

then implement it for RegularPath, SerializedPath (or names you choose). This would let us return F::Result from find_path and handle both regular results which are Vec<Step>s and serialized results which are Strings.

Wouldn't it be easier just to make a Path enum with a SerializedPath(String) and a VectorizedPath(Vec<Step>) variant?

It would be easier, but it then someone who only wants non-serialized paths would have to unwrap the result. Since it's in our power to give type-system-level assurance that the path is a given type, I'd rather do that than require extra runtime checks.

I mean it's fine if we don't handle non-serialized paths for now as long as we don't allow someone to set the 'serialized' FindOptions setting.

Having an enum is still good though! I'd just rather have the type-level assurance that the user gets the correct type as well.

I've preemptively opened #61 for implementing this after this PR is merged.

ASalvail · 2018-09-24T22:42:31Z

Unfortunately the lifetime shenanigans are related to having a closure at all that we pass into JavaScript which is not 'static. I think we'll need it any time we want to have full capability in callbacks.

In the closure example from stdweb's doc, none of this is mentioned. I don't believe that it the closure there is anymore static. I'm confused about whether it's a document failure on their end or if it's a problem particular to our use case that requires the whole unpacking of the callback.

daboross · 2018-09-24T23:11:31Z

You're right, it isn't mentioned there. The page doesn't explicitly mention that you can use non-static data either, though. It just mentions "directly embedding rust code" which is fully doable with only static closures.

I think I came to the conclusion just by testing it out and seeing that it didn't work. For example, if you modify the main page's example to use non-static data, it will fail:

#[macro_use]
extern crate stdweb;

fn main() {
    let outer_owned_variable = "Goodbye.";
    let print_hello = |name: String| {
        println!("Hello, {}! {}", name, &outer_owned_variable);
    };

    js! {
        var print_hello = @{print_hello};
        print_hello( "Bob" );
        print_hello.drop(); // Necessary to clean up the closure on Rust's side.
    }
}

$ cargo check
    Checking stdweb-test v0.1.0 (/home/daboross/stdweb-test)                                                                                                 
error[E0597]: `outer_owned_variable` does not live long enough                                                                                               
  --> src/main.rs:7:42                                                                                                                                       
   |                                                                                                                                                         
6  |     let print_hello = |name: String| {                                                                                                                  
   |                       -------------- value captured here                                                                                                
7  |         println!("Hello, {}! {}", name, &outer_owned_variable);                                                                                         
   |                                          ^^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough                                                  
...                                                                                                                                                          
15 | }                                                                                                                                                       
   | - `outer_owned_variable` dropped here while still borrowed                                                                                              
   |                                                                                                                                                         
   = note: borrowed value must be valid for the static lifetime...                                                                                           
                                                                                                                                                             
error: aborting due to previous error                                                                                                                        
                                                                                                                                                             
For more information about this error, try `rustc --explain E0597`.                                                                                          
error: Could not compile `stdweb-test`.                                                                                                                      

To learn more, run the command again with --verbose.

We could mirror the stdweb behavior and not accept closures with borrowed data. This would be fully doable, we could eliminate all of the extra code and greatly simplify PathFinder bindings.

If we did this, though, our bindings would not allow closures which borrow local data. I made the decision to do all of the extra stuff simply because using borrowed data is generally a useful thing to do.

ASalvail · 2018-09-24T23:35:13Z

Now that you mention it, I remember you telling me exactly this, but I suppose I'm a very forgetful person!

This is an amazing explanation however, we should put it in the documentation somewhere...

ASalvail · 2018-09-24T23:42:56Z

Waiting for a second pass through the code.

daboross

This looks good- good work on the callback bindings!

I've written a few notes, then I think we do need a custom Deserialize implementation for Direction.

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

daboross · 2018-09-25T00:16:19Z

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

+
+#[derive(Debug, Deserialize)]
+pub enum Path {
+    VectorizedPath(Vec<Step>),


Path in the variant names seems redundant here- it's already in the Path enum?

I think Path::Vectorized would be just as clear as Path::VectorizedPath.

Very good idea.

daboross · 2018-09-25T01:44:52Z

If you want, I could merge this PR as is and just merge #56 on top of it afterwards. Might be nice in order to have this with all the comments officially marked as "merged"?

ASalvail · 2018-09-25T01:52:02Z

Please do, I'll finish #56 later

daboross · 2018-09-25T02:06:56Z

👍

thanks!

ASalvail mentioned this pull request Sep 21, 2018

Implements methods related to paths #56

Merged

daboross reviewed Sep 21, 2018

View reviewed changes

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

ASalvail added 2 commits September 24, 2018 18:44

Implements Room.findPath()

24074a6

Fixes use path

af50125

Support for overwriting the CostMatrix

8e241d5

Supports return value of both serialized and vectorized path

8487819

daboross requested changes Sep 25, 2018

View reviewed changes

daboross mentioned this pull request Sep 25, 2018

Type-level assurance of path being serialized or not in Room::find_path #61

Open

ASalvail added 3 commits September 24, 2018 21:07

Removed Path stutter

a26c287

Changed variants name and made the deserialization untagged

b9b312f

Export enums' name into the objects namespace

b2b9433

daboross approved these changes Sep 25, 2018

View reviewed changes

ASalvail changed the title ~~[For review, not to be merged] Implements Room.findPath()~~ Implements Room.findPath() Sep 25, 2018

daboross merged commit 8f20cfe into rustyscreeps:master Sep 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements Room.findPath() #53

Implements Room.findPath() #53

ASalvail commented Sep 21, 2018

ASalvail commented Sep 21, 2018

daboross commented Sep 21, 2018

daboross left a comment

daboross Sep 21, 2018

ASalvail Sep 24, 2018

daboross Sep 21, 2018

daboross Sep 21, 2018

ASalvail Sep 24, 2018

daboross Sep 25, 2018

daboross Sep 25, 2018

ASalvail commented Sep 24, 2018

daboross commented Sep 24, 2018 •

edited

Loading

ASalvail commented Sep 24, 2018

ASalvail commented Sep 24, 2018

daboross left a comment

daboross Sep 25, 2018

ASalvail Sep 25, 2018

daboross commented Sep 25, 2018

ASalvail commented Sep 25, 2018

daboross commented Sep 25, 2018

Implements Room.findPath() #53

Implements Room.findPath() #53

Conversation

ASalvail commented Sep 21, 2018

ASalvail commented Sep 21, 2018

daboross commented Sep 21, 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

ASalvail commented Sep 24, 2018

daboross commented Sep 24, 2018 • edited Loading

ASalvail commented Sep 24, 2018

ASalvail commented Sep 24, 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

daboross commented Sep 25, 2018

ASalvail commented Sep 25, 2018

daboross commented Sep 25, 2018

daboross commented Sep 24, 2018 •

edited

Loading