Switch to lodash for memory lookup

[ASalvail]

While the parameters name led me to believe that I could dereference paths using the obj[path] syntax, I couldn't make it work. However, during my search, I did discover lodash's get method that actually does that. This means that instead of

let memory = screeps::memory::root();
let val = memory.dict("creeps").unwrap().dict("John").unwrap().int("time").unwrap();

now we can directly query any layers of the memory tree:

let memory = screeps::memory::root();
let val = memory.int("creeps.John.time");

This is not quite native structs, but it is a lot less cumbersome.

[daboross]

Definitely less cumbersome! Still slightly more overhead than I'd like to include, though, and lodash is much more accepting of invalid things than I'd prefer. Not that this crate is high performance in the first place, but it is nice to have minimal access available.

What would you think of using a macro to do this kind of access?

let memory = screeps::memory::root();
let val = memory_get!(int, memory.creeps.John.time);
let val2 = memory_get!(int, memory.creeps[name].time);
// translating into
let val = memory.dict("creeps").and_then(|v| v.dict("John")).and_then(|v| v.int("time"))
let val = memory.dict("creeps").and_then(|v| v.dict(name)).and_then(|v| v.int("time"))

It'd be slightly more verbose, but possibly easier to include include our own values. I'd also really like to keep at least one interface into Memory with minimal overhead in case anyone wants to use it.

This might be a bit hypocritical, but I think it'd be alright to include a utility macro like this in screeps-game-api as long as the original interface remains there.

Edit: I can work on implementing this macro, since I think it's a useful thing.

---

I'd also be open to introducing the changes you've made here too as long as we can keep a non-lodash-using Memory interface alongside the one which does.

Thoughts?

[ASalvail]

Still slightly more overhead than I'd like to include

Because of the use of lodash? I thought to use it after I saw it used in one of the creep's method, IIRC.

Still, I am completely fine with the macro with two caveats:

• I assume making a single call to js is better than multiple ones (ie, I expect that going through js! isn't a zero-cost abstraction. In which case, I don't know whether I prefer the macro over _.get.
• Parsing those strings is going to be really non-trivial.

It all boils down to what you have in mind as a FFI to the screeps API.

My first thought was to completely skip Memory and process RawMemory ourselves which would side-step the Reference headaches. It turned out to be harder than I expected. While I'm not ready to write it off as impossible, it's maybe not the best use of our time now.

Now, I thought that going the route of emulating the way the memory access is done in screeps as closely as possible would be a better way to go, but I get the concerns of overhead.

Making a whole other struct for lodash support seems a bit too heavy, but I'd like having two versions of each methods, one lodash, the other vanilla. But pretty please, don't call the vanilla parameters path. I spent more time than I care to admit trying to find a way to encode a full path.

So my suggestion would look like:

    pub fn num(&self, key: &str) -> Option<f64> {
        (js! {
            return (@{self.as_ref()})[@{key}];
        }).try_into()
            .map(Some)
            .unwrap_or_default()
    }

    pub fn path_num(&self, path: &str) -> Option<f64> {
        (js! {
            return _.get(@{self.as_ref()}, @{path});
        }).try_into()
            .map(Some)
            .unwrap_or_default()
    }

with maybe an eventual rewrite using a macro to avoid using lodash.

How's that?

[daboross]

That'd be good!

The only reason to avoid lodash here is parsing strings at runtime which could instead be separated at compile time, but I wasn't thinking about crossing the rust<->js boundary multiple times. It's my understanding that it's almost as cheap as a regular JS function call since node runs WASM within the same runtime as JS, but there still could be overhead.

Writing the macro itself shouldn't be too much work if we did want to go that path. macro_rules! will do most if not all of the code parsing for you, and I think a strategy like incremental TT munchers would fit well.

[ASalvail]

Let's put this PR on the ice while you have a look at the macro then!

[daboross]

Alright- I'm still not 100% convinced the macro is the best idea, but it is simple-ish to implement.

While function calls might be trivial, I'd forgotten that that also involve sending strings across the rust<->js boundary, which might be less trivial.

What would you think of having both as alternatives?

The macro could be nicer to use if there's variables included, but I have no idea which is more performant, and the macro syntax isn't necessarily as nice as including a string.

[ASalvail]

I'm fine having more choices rather than fewer. We should take some care to document this discussion however.

[ASalvail]

How about switching from Option<T> to Result<T, ConversionError>?
It'd mean that we could interact with the memory with ?, which I believe is the more rustic way of doing things?

[ASalvail]

Another thing that came to me: should we rename num to float?

[daboross]

This probably deserves some discussion on what the best method is.

I originally used = undefined since it's strictly faster (or at least was in the V8 version that was benchmarked a while ago), but it's true that it doesn't exactly represent the right semantics.

[ASalvail]

I though it would just store an empty object, but it turns out that it does get rid of it! Maybe a result of the serialization process.

In that case, I can revert the last commit!

[daboross]

If we're going to return conversion errors, I think it'd be most appropriate to return Option<Result<T, ConversionError>> since it can also fail by not having a value. That sounds like a good idea, though.

Re: float: I'm not 100% sure on what the naming convention should be. We could go all the way to the rust side on naming conventions and rename them to get_i32, get_f32, ` etc.

[ASalvail]

My preference would be for i32 (or whatever js uses as integers) and f64. It has the merit of being very clear.

For the conversion to Result I'd wait later, it's not urgent.

[ASalvail]

For the Result<Option<T>, ConversionError>, could I convince you to go with something like Result<T, MemoryError>, with

enum MemoryError {
  ConversionError,
  UndefinedError
}

I understand the logic behind having an unexpected error and an expected error, but I would argue that you would never try and fetch memory that you know is undefined. Plus, it's less unwieldy to use.

Thoughts?

[daboross]

I'd argue fetching memory possibly undefined is almost one of the most common operations? At least in my experience it's much more common than fetching a value you know to be defined.

I mean for a lot of values, undefined reasonably means "whatever the default is". Take a simple example with a creep storing whether it's currently gathering energy. The most efficient way to store this would be to have gathering: true mean the creep is gathering, and "gathering not present" mean it's not. You could store a false value, but it'd be semantically useless and it would cost precious extra CPU to parse at the beginning of each tick.

For many other things, they might be uncommonly stored. Like creep.memory.running_from_enemy for example could store a RoomPosition which the creep last saw an enemy creep it's currently running away from. For safety the creep should continue running away at least a room's distance, and at that point the enemy is no longer in view- thus it's stored in memory. This memory value would have no reasonable default, and it too would be a waste to store in every creep's memory, but every creep would want to check for it at the beginning of the tick to ensure it's not currently running away. There are certainly other ways to do this, but that's not an uncommon strategy for this or for many other optional states a creep could be in.

For memory values stored globally, sure, it does make sense to always have a value and initialize it at start. But even then you'd want to be able to delete that memory manually in the console if it's somehow gotten bad data and have the AI reinitialize it with the default.

In all of these cases I would argue "not present" is a fairly normal case, whereas "wrong type of value" is an irreparable error. If creep.memory.gathering is not a bool, there is no reasonable course of action, especially if it's because some other data was stored there that we don't want to clobber.

Hope that perspective is helpful? Let me know what you think.

[ASalvail]

That makes a lot of sense! This is where it shows I don't know the game very well.

Now, here is partly why I am asking. Look at this:

    fn try_convert<T : TryFrom<Value> + AsRef<Reference>>(&self, key: &str) -> Result<Option<T>, <T as TryFrom<Value>>::Error> {
        let v = js!{ return @{self.as_ref()}[@{key}]; };
        v.try_into()
    }

I thought this would be a very reasonable function. However, the compiler gives me this very helpful error:

error[E0271]: type mismatch resolving `<enum_primitive::Option<T> as stdweb::unstable::TryFrom<stdweb::Value>>::Error == <T as stdweb::unstable::TryFrom<stdweb::Value>>::Error`
  --> screeps-game-api\src\memory.rs:95:11
   |
95 |         v.try_into()
   |           ^^^^^^^^ expected enum `stdweb::private::ConversionError`, found associated type
   |
   = note: expected type `stdweb::private::ConversionError`
              found type `<T as stdweb::unstable::TryFrom<stdweb::Value>>::Error`

error[E0271]: type mismatch resolving `<T as stdweb::unstable::TryFrom<stdweb::Value>>::Error == stdweb::private::ConversionError`
  --> screeps-game-api\src\memory.rs:95:11
   |
95 |         v.try_into()
   |           ^^^^^^^^ expected associated type, found enum `stdweb::private::ConversionError`
   |
   = note: expected type `<T as stdweb::unstable::TryFrom<stdweb::Value>>::Error`
              found type `stdweb::private::ConversionError`
   = note: required because of the requirements on the impl of `stdweb::unstable::TryFrom<stdweb::Value>` for `enum_primitive::Option<T>`
   = note: required because of the requirements on the impl of `stdweb::unstable::TryInto<enum_primitive::Option<T>>` for `stdweb::Value`

This is all the more annoying given that it's perfectly fine if I do:

    fn try_convert(&self, key: &str) -> Result<Option<String>, <String as TryFrom<Value>>::Error> {
        let v = js!{ return @{self.as_ref()}[@{key}]; };
        v.try_into()
    }

Any clue what I could be missing? Stdweb seems to have been to great length to ensure you cannot put your hand on their ConversionError type. I'm starting to believe I should either ignore their error type and build ours crate wide or somehow try to wrap it (maybe boxing it 🤔 )

[daboross]

@ASalvail Yeah, stdweb hiding ConversionError is fairly annoying.

I see you've got the <xx as TryFrom<Value>>::Error thing working with the string try_convert.

To get the generic one, I think you need to require that T's error type is ConversionError. Using that same trick should work, so I think the bound would be T: TryFrom<Value, Error=<String as TryFrom<Value>>::Error>?

[ASalvail]

Seems like it is related to rust-lang/rust#34430
I opened #39 to address the issue regarding ConversionErrors.

[ASalvail]

Using T: TryFrom<Value, Error=<T as TryFrom<Value>>::Error> won't work as this becomes a circular reference. It so happens that everything would work by using String instead of T to qualify the parent trait of the Error, but that's a smelly hack.

[daboross]

@ASalvail Yeah- I definitely mean using String in the inner position. What is being said here is that T's error type needs to match String's.

I've opened koute/stdweb#280 to see if stdweb has any interest in helping with this annoyance.

[ASalvail]

Alright, I abandoned (temporarily!) my big plans for conversion errors that make sense. I'd rather come back to it later once we get some insight on koute/stdweb#280 and koute/stdweb#281 which makes it very difficult to have Result<Option<T>, E> and merge this meanwhile.

[daboross]

Looking good!

I do think we should keep the checks to prevent returning arrays as MemoryReference references, but the rest of it's good.

[daboross]

Just for style, I'd prefer longer generic requirements be written in a where clause rather than being linline.

Something like this separates things out a bit more cleanly, and definitely wraps better:

pub fn get<T>(&self, key: &str) -> Result<Option<T>, ConversionError>
where
    T: TryFrom<Value, Error = ConversionError> + AsRef<Reference>,
{

}

[daboross]

Without the array check this will incorrectly succeed on arrays since they are References.

Don't need the whole v || null thing (not sure why I felt the need to replace undefined with null), but the if (_.isArray(v)) check is necessary to prevent incorrectly returning a MemoryReference wrapping an array.

[ASalvail]

Actually, that should be taken care of by the defn of TryFrom<Value> for MemoryReference given later since I'm not wrapping the MemoryReference around the Reference myself.

[daboross]

Unrelated to arrays: I think this was meant to be a non-path lookup?

[ASalvail]

Good catch!

[ASalvail]

I addressed your previous comment. Regarding the check for the _.is_array(), I believe that your implementation of TryFrom<Value> for MemoryReference has us covered since I'm not doing the wrapping myself.

[daboross]

That's a good point- I totally missed that. Thanks!