-
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
Parameterize scenario datatypes #903
Conversation
{ cellTerrain :: TerrainType | ||
, cellEntity :: Maybe Entity | ||
, cellEntity :: Maybe e |
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 the idea is that instead of writing something like:
worldMapToText :: WorldPalette -> [[Cell]] -> Text
we would instead do it in two steps:
simplifyEntityForMe :: Entity -> SimplerEntity
simpleWorldMapToText :: WorldPalette (PCell SimplerEntity) -> Text
Is this because you want to have ToJSON
(not ToJSONE
) instance? 🤔
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.
@kostmo if this is about ToJSON
we discussed with @byorgey, that you could simply use a custom tuple:
newtype Env e v = Env (e, v)
instance ToJSON (Env E Val) -- maybe we could codify this pattern as ToJSONE => ToJSON (Env, Val)
which is the same as
simplify :: E -> Val -> SimpleVal
instance ToJSON SimpleVal
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.
Is this because you want to have ToJSON (not ToJSONE) instance?
I haven't quite wrapped my head around how FromJSONE
works (or how ToJSONE
would).
For the purpose of the World Editor, we just need to emit a fragment of the scenario file (either to a file or via the web API ).
I'm not sure how easy it is to read in its current state, but here's what I'm trying to do in the World Editor PR.
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.
After reading through your PR, I think it makes sense to keep it simple. 👍
The idea with FromJSONE
is that you need the environment to choose between parsing alternatives.
But with ToJSON
no tricks are necessary, so I was incorrect in thinking that we would ever need it.
beabffd
to
be9a6ce
Compare
src/Swarm/Game/State.hs
Outdated
@@ -144,6 +144,7 @@ import Swarm.Game.Recipe ( | |||
reqRecipeMap, | |||
) | |||
import Swarm.Game.Robot | |||
import Swarm.Game.Scenario.WorldDescription |
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.
Why do we need this new import?
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.
Huh, I would have expected a compiler warning about an unused import. Anyway, I've removed it and it still works.
@@ -52,7 +56,7 @@ instance FromJSONE (EntityMap, RobotMap) WorldDescription where | |||
-- string into a nested list of 'Cell' values by looking up each | |||
-- character in the palette, failing if any character in the raw map | |||
-- is not contained in the palette. | |||
paintMap :: MonadFail m => WorldPalette -> Text -> m [[Cell]] | |||
paintMap :: MonadFail m => WorldPalette Cell -> Text -> m [[Cell]] |
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.
Can we make this polymorphic, WorldPalette c -> Text -> m [[c]]
or something like that?
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.
done
newtype WorldPalette = WorldPalette | ||
{unPalette :: KeyMap Cell} | ||
newtype WorldPalette c = WorldPalette | ||
{unPalette :: KeyMap c} |
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.
Would it be better to have newtype WorldPalette e = WorldPalette { unPalette :: KeyMap (PCell e) }
? I am honestly not sure. Or maybe we should just double down on the generality and call it Palette
instead of WorldPalette
--- it maps characters to anything.
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.
fixed.
be9a6ce
to
ccdc763
Compare
Towards #873
Motivation
The
Entity
datatype has a fair amount of state and extra fields that are not relevant to serializing a scenario (in particular, a world map) to a file.In this PR, the
Cell
record is renamed toPCell
(representing "Parameterized Cell"), replacingEntity
with a type parameter. This permits the introduction of a trimmed-downEntity
type for the purpose of serialization. This lightweight type can also be a reliableMap
key.A type alias
Cell
is created with the originalEntity
type as a parameter.The
WorldDescription
andWorldPalette
are likewise parameterized on theCell
type.