-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
- Add robotID and robotParentID to every robot - Add special 'self' and 'parent' variables
Sets a robot's display name. The display name is no longer used to uniquely identify a robot; it only exists as a convenience for players.
`x <- c` is just desugared to `c >>= \x -> return x`
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.
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>
There was a problem hiding this 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.
-- 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Ah, good point.
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. |
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.
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>
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. |
addRobot r = do | ||
rid <- gensym <+= 1 | ||
r' <- case r ^. robotID of | ||
(-1) -> do |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
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.
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.
The basic idea of this change is to create a new
robot
type and use it to identify robots instead ofstring
names. Internally, arobot
value is just a (unique)Int
.Closes #212 .
This ended up turning into a sort of constellation of related changes.
robot
type and change the type of various built-in functions which used to take a robot name so they now take arobot
(give
,install
,reprogram
,view
,upload
) and changebuild
so it returns arobot
.Int
) robot ID rather than by name.setname
command for setting a robot's display name (which no longer needs to uniquely identify a robot).base
,parent
, andself
for getting arobot
value referring to the base, one's parent, and one's self, respectively.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.Value
by doubling down on our current strategy of injectingValue
s back intoTerm
s 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 ofTerm
which represent internal results of evaluation and cannot show up in the surface language (TRef
,TRobot
).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
andIntSet
instead of looking things up byText
keys in aMap
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.