Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a new opaque type for robots instead of strings #303

Merged
merged 26 commits into from
Mar 2, 2022
Merged

Conversation

byorgey
Copy link
Member

@byorgey byorgey commented Feb 1, 2022

The basic idea of this change is to create a new robot type and use it to identify robots instead of string names. Internally, a robot value is just a (unique) Int.

Closes #212 .

This ended up turning into a sort of constellation of related changes.

  • Add the robot type and change the type of various built-in functions which used to take a robot name so they now take a robot (give, install, reprogram, view, upload) and change build so it returns a robot.
  • All internal data structures that store robots are now keyed by a unique (Int) robot ID rather than by name.
  • Add a setname command for setting a robot's display name (which no longer needs to uniquely identify a robot).
  • Import a big list of words which we can use to randomly pick names for robots, just for fun. This is why the diff says "+31,050 -265"; I did not write 31 thousand lines of code.
  • Add constants base, parent, and self for getting a robot value referring to the base, one's parent, and one's self, respectively.
  • Top-level binders like r <- build {move} now export a variable binding which can be used in later expressions entered at the REPL; additionally, unlike Haskell, a binder can now appear as the last statement in a block.
  • Fix the pretty-printer for Value by doubling down on our current strategy of injecting Values back into Terms and then pretty-printing the result. I am now convinced this is the Right Way (tm) to do this; it only required adding a couple additional kinds of Term which represent internal results of evaluation and cannot show up in the surface language (TRef, TRobot).
  • Update the tutorial.
  • While updating the tutorial, I noticed that homomorphic hashing for inventories #294 had introduced a bug, where the inventory display no longer updated when 0 copies of an entity are added to the inventory (as with scan + upload), so I fixed that by changing the way inventory hashes are computed.

I tried running the benchmarks both before & after this change. I was hoping that it might speed things up to be using IntMap and IntSet instead of looking things up by Text keys in a Map all the time. However, if I'm interpreting the results correctly, it seems like it didn't really make all that much difference, at least for the particular benchmarks we have.

@byorgey
Copy link
Member Author

byorgey commented Feb 14, 2022

I'm thinking a nice way to do the "access past REPL results" thing would be to have an special primitive array available which is notionally appended to every time a new REPL value is returned (which of course would depend on #98 ). We could change the REPL output to show the index, e.g.

> 3 + 5
res[0] = 8
> build {move}
res[1] = <r0>
> def r = res[1] end

or somerthing like that.

The base's inventory was not updating when a robot uploaded some scan
data to it.  The problem was that after switching to homomorphic
hashing for inventories (#229),
adding 0 copies of something to the inventory, although it still
worked properly, no longer caused the inventory hash to be updated,
and the UI uses the hash to decide when to redraw.

The solution was to let each entity with k copies in the inventory
contribute (k+1) times its hash to the inventory hash.  This way the
hash distinguishes between having 0 copies of an entity and not knowing
about it at all.
Co-authored-by: Restyled.io <commits@restyled.io>
@byorgey byorgey marked this pull request as ready for review February 15, 2022 21:42
@byorgey byorgey requested a review from polux February 15, 2022 23:22
Copy link
Collaborator

@polux polux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! One thing that you may want to consider: the word list contains many words that may be offensive or traumatic to some people. I would suggest (but this is additional work) curating a list of funny-sounding robot names instead, and then add some counter to them once you've reached the end. Appending the counter doesn't have to be costly: the list of names can be an infinite list of name/number pairs.

TUTORIAL.md Outdated Show resolved Hide resolved
-- the ID as a key, etc. In practice, the ID will be set once, when
-- adding the robot to the world for the first time, and then never
-- touched again.
unsafeSetRobotID :: RID -> Robot -> Robot
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we modify mkRobot to accept an id instead? Or have a monadic version of mkRobot that bumps the gensym? It feels a bit weird that mkRobot lets you create a robot that's missing a required field.

Alternatively: Robot could be parameterized by its id type, and mkRobot would create a Robot (). Or it could be parameterized by a phase, à la trees that grow. Maybe it would help with typing the robots' lifecycle beyond the creation case. It's probably overkill.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good point, I agree this feels weird. I don't remember why I did it this way. I'll give it some more thought and see if I can either come up with a better way to do it or at the very least an explanation of why I think it should be done this way after all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mkRobot now accepts an ID. We still do need unsafeSetRobotID, but only in one very specific circumstance: when parsing a Robot from a JSON file we don't have access to a State GameState effect so we can't generate a unique ID at parse time, and have to fill one in later when adding the robot to the world.

@byorgey
Copy link
Member Author

byorgey commented Feb 16, 2022

the word list contains many words that may be offensive or traumatic to some people

Ah, good point.

I would suggest (but this is additional work) curating a list of funny-sounding robot names instead, and then add some counter to them once you've reached the end.

Good idea. I think I should be able to come up with a curated list somehow. And there's actually no issue with recycling names, since the names no longer have to be unique.

@TristanCacqueray
Copy link
Collaborator

Here is another (safer) way to get funny names: https://github.com/moby/moby/blob/master/pkg/namesgenerator/names-generator.go.

There is also https://github.com/fakedata-haskell/fakedata#readme , which features really nice data, e.g. from futurama, street fighter or back to the future. Unfortunately the package has many modules and it takes quite a bit of time to compile.

I didn't catch this at first, and in fact while making these updates I
found at least one other bit of example code in an entity description
which was outdated and had gone unnoticed for a while.  It would be
really cool if somehow we could specially mark any code examples in
descriptions, and have them automatically checked as part of the CI.
@byorgey
Copy link
Member Author

byorgey commented Feb 28, 2022

For generating random robot names, I'm thinking of just creating a module based on https://github.com/moby/moby/blob/master/pkg/namesgenerator/names-generator.go which @TristanCacqueray linked to (with appropriate attribution + license of course).

Co-authored-by: Restyled.io <commits@restyled.io>
@byorgey
Copy link
Member Author

byorgey commented Mar 1, 2022

I think I've addressed all the feedback now! We could certainly think about adding more adjectives and names to the lists used to generate robot names, but this should be good for starters. I'll merge in a bit unless anyone has additional comments.

@byorgey byorgey added the merge me Trigger the merge process of the Pull request. label Mar 2, 2022
@mergify mergify bot merged commit b62d27e into main Mar 2, 2022
@mergify mergify bot deleted the robot-type branch March 2, 2022 03:00
addRobot r = do
rid <- gensym <+= 1
r' <- case r ^. robotID of
(-1) -> do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, late to the party, and maybe not worth it, but it seems to me this is a somewhat hacky way to encode a (Maybe Nat). Should we maybe use a Maybe Nat instead as the robot ID?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you're right, it is hacky. But I am reluctant to change the robot ID to Maybe Nat because it would mean having to annoyingly deal with the Nothing possibility in many, many places in the code where it is not supposed to be possible.

Hmm, maybe it would actually be worthwhile doing something like what you suggested before, where the Robot type is somehow indexed to show whether it has a robot ID or not (i.e. keeping track of the "robot lifecycle"). Something like this, perhaps?

data RobotRecord f = RobotRecord { ..., _robotID :: f Int, ... }
type Robot = RobotRecord Identity
type URobot = RobotRecord Maybe  --- U for Unidentified
mkRobot :: Int -> ... -> Robot
mkURobot :: ... -> URobot

I'd have to think more carefully about where each of these types would go, and how they would interact with the places we are currently using a robot ID of (-1).

Copy link
Member Author

@byorgey byorgey Mar 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I started doing this refactoring, changing the Robot type to track whether or not it had an ID number at the type level, then just fixing type errors one by one to complete the refactoring. Most of the errors were of the usual "refactoring" kind: changing all the places I was referring to a name that is now called something different, etc. But then at one point it threw up a type mismatch error, saying that it was expecting a certain list to have the type [Robot] (i.e. robots with IDs) but it actually had the type [URobot] (robots without IDs)... and it was actually a bug! We weren't assigning ID numbers to robots read from challenge descriptions, so in challenges with multiple robots all but one would instantly disappear (since they all shared the ID number of -1). It is now impossible to add a robot without an ID number to the world, because the types simply don't match. (Of course it is still theoretically possible to add multiple robots with the same ID, but in practice you have to add robots via the addURobot function which will automatically assign a fresh ID).

A big thank you to @polux for bringing me to my senses. I will open a PR with the changes shortly.

byorgey added a commit that referenced this pull request Mar 4, 2022
See the discussion at
#303 (comment) .
This seems like an unqualified success: no more hacky (-1)'s, and
doing the refactoring actually uncovered a bug!  Previously, we were
not actually assigning ID's to the robots that were read as part of a
challenge.  This means that in a challenge with multiple robots, all
but one of them would instantly disappear since they all shared the
same ID number.
byorgey added a commit that referenced this pull request Mar 6, 2022
See the discussion at
#303 (comment) .
This seems like an unqualified success: no more hacky (-1)'s, and
doing the refactoring actually uncovered a bug!  Previously, we were
not actually assigning ID's to the robots that were read as part of a
challenge.  This means that in a challenge with multiple robots, all
but one of them would instantly disappear since they all shared the
same ID number.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opaque robot type
3 participants