September 2018 game update #71

ASalvail · 2018-09-29T18:36:20Z

Implements the elements from the update described in this changelog.

Except the sign string. This will be part of a review of all the constants that I plan to do a bit later.

daboross

Thanks for this!

I've included some comments - we can discuss things in them?

I'd be fine if we use the no-argument form of getRawBuffer, and I can make that more efficient in a later PR.

daboross · 2018-09-29T23:37:22Z

screeps-game-api/src/constants.rs

@@ -250,6 +250,9 @@ pub enum Terrain {
    Plain = 0,
    Wall = 1,
    Swamp = 2,
+    SwampWall = 3,


As it is right now I don't see a reason to include this separately from Wall.

It makes no gameplay difference whether a wall has a swamp underneath it or not, and forcing consumers of Terrain to consider that there is a difference seems a bit wrong?

o4kapuk has also mentioned on slack that the official server has gone through and replaced all terrain 3 tiles with terrain 1, and that the screeps team is working on a patch for the private server to do a similar thing.

Oh, I didn't know that. Fair enough, I'll remove it.

daboross · 2018-09-29T23:40:23Z

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

@@ -102,10 +104,19 @@ impl Room {
        }
    }

+    pub fn get_event_log(&self) -> Vec<Event> {
+        let raw_event_log: String = js_unwrap!{@{self.as_ref()}.getEventLog(true)};


To fully fix #59 it'd be nice to have a get_event_log_raw function returning this String directly as well.

I really don't see the usage, but sure.

Maybe storing the event log in a segment for further inspection? I could imagine doing this when in a "low cpu available" mode if under attack and an exact replay of events is wanted. There's the room history, but that's not entirely reliable, and the AI itself cannot analyze it.

Or an AI could want to use something besides serde_json to deserialize events, possibly skipping irrelevant ones to save CPU.

I'm not sure I would do any of these things, but since the JS API does give us this option and it is much cheaper than parsing it, I'd prefer giving that option as well.

daboross · 2018-09-29T23:42:37Z

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

+pub struct Event {
+    #[serde(flatten)]
+    pub event: EventType,
+    #[serde(rename = "objectId")]


Would this work as #[serde(rename_all = "camelCase")] on the type? Just to match the other structures and to not single this one out.

daboross · 2018-09-29T23:44:28Z

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

+#[derive(Clone, Debug, PartialEq, Eq, Deserialize)]
+#[serde(tag = "event", content = "data")]
+pub enum EventType {
+    #[serde(rename = "1")]


Does this work?

I have not tested whether serde equates the string "1" with the JSON integer 1 for tags like this. If you have, it's all good.

I'd almost want us to include a small unit test to ensure EventType deserializes correctly in the code, but for now if we know it works that's enough.

I have not tested it. My problem is working on getting those to actually make sense without, but I might have to write a deserializer manually for this one. I had forgotten that integers were a thing in JSON.

It is actually being worked on right now:
serde-rs/serde#1392

I might wait a few days and see whether this goes anywhere. Still, I'm learning how to manually write a deserializer in the meantime.

daboross · 2018-09-30T00:10:06Z

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

+    }
+
+    pub fn get_raw_buffer(&self) -> Vec<u8> {
+        js_unwrap!(@{self.as_ref()}.getRawBuffer())


As mentioned in my comment, I think it should be pretty simple to create the Vec<u8> ourselves and ask the game to populate it rather than having the game create it and then copy it into WASM.

daboross · 2018-09-30T00:13:13Z

screeps-game-api/src/constants.rs

@@ -250,6 +250,9 @@ pub enum Terrain {
    Plain = 0,
    Wall = 1,
    Swamp = 2,
+    SwampWall = 3,
+    Lava = 4,


I don't think this should be necessary? I'm against including if if it doesn't occur just because it forces any consumers of Terrain to insert an extra case Terrain::Lava => unreachable!(), whenever matching on it which will never be triggered.

The screeps team has mentioned on slack that TERRAIN_MASK_LAVA has never been used, and that there are no current plans for using it. It's also missing from Room.Terrain.get's listed output.

It's a question of whether you prefer to implement 1:1 the constants or clean-up the API on our end. I'm all for making the usage easier rather than going for 1:1 translation.

In this case I would go for "cleanup", but I think 1:1 would agree here too. We're only ever returning the terrain, never passing it into the game, and the only place it's returned is documented to not return 4. Even though the constant exists, it's documented to never be used?

ASalvail · 2018-09-30T15:50:45Z

So far, I'm unable to write a sane implementation of a deserializer for Event. Here's a summary of my problem, maybe you could have an idea.

The structure of the event log is defined as

{
    event: EVENT_ATTACK,
    objectId: '54bff72ab32a10f73a57d017',
    data: { /* ... */ }
}

with data depending on the event code, which is a u8 ranging from 1 to 10. To represent this in rust, I have an Event

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Event {
    pub event: EventType,
    pub object_id: String,
}

and the inner EventType

#[derive(Clone, Debug, PartialEq, Eq, Deserialize)]
pub enum EventType {
    Attack(AttackEvent),
    ObjectDestroyed(ObjectDestroyedEvent),
    AttackController,
    Build(BuildEvent),
    Harvest(HarvestEvent),
    Heal(HealEvent),
    Repair(RepairEvent),
    ReserveController(ReserveControllerEvent),
    UpgradeController(UpgradeControllerEvent),
    Exit(ExitEvent),
}

Now, serde_derive provides an attribute #[serde(tag = "event", container = "data")]. This would work if the event field was not a u8. Since this is not a possibility until the aforementioned PR is completed in serde, my option is to manually implement it.

impl<'de> Deserialize<'de> for Event {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        #[derive(Deserialize)]
        #[serde(field_identifier, rename_all = "camelCase")]
        enum Field { Event, ObjectId, Data };

        struct EventVisitor;

        impl<'de> Visitor<'de> for EventVisitor {
            type Value = Event;

            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
                formatter.write_str("struct Event")
            }

            fn visit_map<V>(self, mut map: V) -> Result<Event, V::Error>
            where
                V: MapAccess<'de>,
            {
                let mut event_type = None;
                let mut obj_id = None;
                let mut et = None;

                while let Some(key) = map.next_key()? {
                    match key {
                        Field::Event => {
                            if event_type.is_some() {
                                return Err(de::Error::duplicate_field("event"));
                            }
                            event_type = Some(map.next_value()?);
                        }
                        Field::ObjectId => {
                            if obj_id.is_some() {
                                return Err(de::Error::duplicate_field("objectId"));
                            }
                            obj_id = Some(map.next_value()?);
                        }
                        Field::Data => {
                            if et.is_some() {
                                return Err(de::Error::duplicate_field("data"));
                            }

                            let data: EventType;

                            if let Some(event_id) = event_type {
                                match event_id {
                                    // No clue
                                };
                            } else {
                                return Err(de::Error::custom("Found data before event."));
                            }

                            et = Some(data);
                        }
                    }
                }
                let et = et.ok_or_else(|| de::Error::missing_field("data"))?;
                let obj_id = obj_id.ok_or_else(|| de::Error::missing_field("objectId"))?;

                Ok(Event{event: et, object_id: obj_id})
            }
        }

        const FIELDS: &'static [&'static str] = &["event", "objectId", "data"];
        deserializer.deserialize_struct("Event", FIELDS, EventVisitor)
    }
}

This solution has two problems:

it assumes that the event field comes first. I couldn't figure out how to postpone the parsing of the value associated with the data key until the end of the parsing of the object. I could probably capture it in a serde_json::Value, but I have no clue how to destructure that later on.
Even given the event code, I can't give a type hint to the parser to parse a specific variant. (e.g. cannot write 1 => data = map.next_value::<EventType::Attack>()?.

So, short of interfacing the data with a HashMap, I am short on solutions. Any ideas?

The solution probably involves parsing both the Event and the EventType at once, but then I'm not sure how to use serde to do that...

daboross · 2018-10-05T00:28:14Z

I've been thinking about this and I'm not sure I have any better solutions. We can't always parse the tag first, since it won't always come first, and we need to do something with the data if it comes before.

The best solution I could think of is to deserialize the data itself into a serde_json::Value if it appears before the event type in the JSON. serde_json::Value does use a HashMap internally, but it has the advantage that it itself is a serde::Deserializer so we can deserialize data from it once we do have the event type.

If the event happens before the data, we can avoid that and deserialize the event directly.

I think this should be feasible? It would make the logic more complicated, but it should be able to work, and the custom Deserialize implementation you've written seems like it could be adapted to use this strategy.

As far as I know this is a limitation of how serde works. Deserializers read data in starting at the beginning and ending at the end and the thing being deserialized doesn't have much choice as to how it receives that input data. I would expect serde-derive's implementation of externally-tagged data uses similar buffering, but I'm not sure.

Let me know what you think of this strategy.

ASalvail · 2018-10-05T07:26:49Z

Deserialization is done and works well.
Once the PR regarding deriving from integers is published from serde, we can remove the custom implementation.

daboross

Looking good! I have a few nitpicking things but Deserialize is well implemented and I think this will be quite nice to use when finished.

daboross · 2018-10-05T07:38:27Z

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

+                    }
+                }
+
+                if data_buffer.is_some() && event_type.is_some() && data.is_none() {


I think we should be able to remove the redundancy between these is_some calls and the unwrap() / if let below if we directly match on the values here. What would you think of something like this?

if data.is_none() { if let (Some(data_buffer), Some(event_type)) = (data_buffer, event_type) { ... } }

I had avoided doing that because I didn't want to get 3 levels deep, but that fixes my concern nicely. Looks great!

Cool!

We might even be able to do

if let (Some(data_buffer), Some(event_type), None) = (data_buffer, event_type, data) {

but I'm not 100% sure rust will allow that since we're modifying data inside and lifetimes are lexical (can't wait for non-lexical lifetimes but that's a whole other topic).

daboross · 2018-10-05T07:44:05Z

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

    }
+
+    pub fn get_raw_buffer_array_ref<'a>(&self, buffer: &'a mut Vec<u8>) -> Result<&'a mut Vec<u8>, ReturnCode> {


Just bikeshedding here: what would you think of making this get_raw_buffer_to_vec?

Current name with the _ref suffix implies we'll be returning a cheap reference to something rather than making a new one, which is slightly misleading. There's a precedent for to_xxx when filling an existing buffer with bytes with functions like https://doc.rust-lang.org/std/io/trait.Read.html#method.read_to_string.

👍 to having this as a public alternative to get_raw_buffer though!

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

daboross · 2018-10-05T07:55:35Z

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

+
+                if data_buffer.is_some() && event_type.is_some() && data.is_none() {
+                    let val: serde_json::Value = data_buffer.unwrap();
+                    let err = |_| de::Error::custom("Can't parse Event data.");


Could we include the display representation of the original error here? It'd be much more useful than a static string. Just something like de::Error::custom(format_args!("can't parse event data due to inner error {}", e)) could work? We should also avoid capitalization and an ending period as serde inserts this as part of a larger sentence (or at least that's what custom's documentation states?).

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

daboross

LGTM!

Sorry again for all the extra nitpick comments. I think we've come to a good point here though: it looks great.

ASalvail · 2018-10-06T05:39:38Z

No need to be sorry, I can give as much as I take :P

ASalvail added 5 commits September 29, 2018 11:55

Implement Room.Terrain. Fixes #57

caf4830

Named version of create_construction_site. Fixes #62

c84e80c

Implements unboostCreep. Fixes #60

8254d50

Implements getEventLog. Fixes #59

30dfbd6

Export room events and add derives

6c25313

daboross requested changes Sep 30, 2018

View reviewed changes

daboross added C-enhancement Category: A PR with an enhancement or a proposed on in an issue. A-screeps-game-api labels Oct 5, 2018

ASalvail added 3 commits October 5, 2018 03:20

Fixed deserialization of events using integers

22a6c0b

Removes unused terrain codes

c668487

Implements passing vector to get_raw_buffer

5f09223

ASalvail force-pushed the sept_game_update branch from 5d050b1 to 5f09223 Compare October 5, 2018 07:22

daboross requested changes Oct 5, 2018

View reviewed changes

Addresses @daboross comments

4851cb0

daboross approved these changes Oct 6, 2018

View reviewed changes

ASalvail merged commit 0d14fc4 into master Oct 6, 2018

ASalvail deleted the sept_game_update branch October 6, 2018 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

September 2018 game update #71

September 2018 game update #71

ASalvail commented Sep 29, 2018 •

edited

Loading

daboross left a comment

daboross Sep 29, 2018

ASalvail Sep 30, 2018

daboross Sep 29, 2018

ASalvail Sep 30, 2018

daboross Oct 5, 2018

daboross Sep 29, 2018

daboross Sep 29, 2018

ASalvail Sep 30, 2018

ASalvail Sep 30, 2018

daboross Sep 30, 2018

daboross Sep 30, 2018

ASalvail Sep 30, 2018

daboross Sep 30, 2018

ASalvail commented Sep 30, 2018 •

edited

Loading

daboross commented Oct 5, 2018

ASalvail commented Oct 5, 2018

daboross left a comment

daboross Oct 5, 2018

ASalvail Oct 5, 2018

daboross Oct 5, 2018

daboross Oct 5, 2018

daboross Oct 5, 2018

daboross left a comment

ASalvail commented Oct 6, 2018

		}

		pub fn get_raw_buffer_array_ref<'a>(&self, buffer: &'a mut Vec<u8>) -> Result<&'a mut Vec<u8>, ReturnCode> {

September 2018 game update #71

September 2018 game update #71

Conversation

ASalvail commented Sep 29, 2018 • edited Loading

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

ASalvail commented Sep 30, 2018 • edited Loading

daboross commented Oct 5, 2018

ASalvail commented Oct 5, 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

daboross left a comment

Choose a reason for hiding this comment

ASalvail commented Oct 6, 2018

ASalvail commented Sep 29, 2018 •

edited

Loading

ASalvail commented Sep 30, 2018 •

edited

Loading