-
Notifications
You must be signed in to change notification settings - Fork 52
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
world editor prototype #873
Conversation
3defea0
to
b08eb40
Compare
@kostmo looks nice, but what do you mean by "unambiguous mapping in the scenario file"? A palette is a JSON object, so its keys are unique. |
It might not be a bijection, though; there may be multiple palette entries that produce a given terrain. So I guess some logic/heuristic must be used to choose one of the palette entries when producing the map string. |
This trick facilitates use of `listEnums` and more concise pattern matching. Towards #873.
bd81ab1
to
613f28c
Compare
I have found myself splitting some of the larger modules to get this PR to work. Perhaps I should spread this work across two PRs: the first will have no World Editor code in it---it will just be moving pre-existing code into different modules (i.e. toward #707). This should make the subsequent PR (adding World Editor functionality) easier to review. If I do that, the splits on their own may not make a ton of sense unless viewed from the context of this current PR. In other words, take a look at the "final" PR to see the direction I'm going with those splits. |
Partially towards #707, although `Scenario.hs` is not that large. Primarily this refactoring is in anticipation of #873 (see [comment](#873 (comment))). Just like #879, there are no functional changes in this PR---only code relocation.
Just like #880, this refactoring is in anticipation of #873 (see [comment](#873 (comment))) for ease of review, and there are no functional changes in this PR. Also towards #707.
4ad962e
to
c97ee55
Compare
Towards #873. * Removes the `Eq` instance for `ModalType` --- I plan to add another member to that enum that contains data without an `Eq` instance. * Decompose `toggleModal` function
faf6263
to
75656a3
Compare
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.
Cool, I think I understand the reasoning better after reading through this PR. 👍
Can't wait to use it to create new scenarios. 🎨 👨🎨
@kostmo sorry it's taken me a while to get to it, but I'm now slowly making my way through this PR. I have some suggestions, but ultimately I think merging it as a feature toggle makes a lot of sense. |
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 it's taken me so long to look at this. In general I agree that we should try to merge this, perhaps at first in such a way that it's disabled by default. I think my biggest meta-comment is that this is a lot of code with very few comments, which made it difficult to review. I lost steam somewhere in TUI.Editor.Controller
--- it was a lot of work to try to figure out what all the code was for. I'm not so much looking for comments that explain what the code does, but comments explaining things like purpose, design choices, where the code fits into the big picture, etc.
toKey :: TerrainEntityFacadePair -> TerrainEntityNamePair | ||
toKey = fmap $ fmap (\(EntityFacade eName _display) -> eName) | ||
|
||
getUniquePairs :: [[CellPaintDisplay]] -> M.Map TerrainEntityNamePair TerrainEntityFacadePair |
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.
What is the purpose of this function? In what sense are the pairs "unique"? It looks to me like if there are multiple things in the input with the same key, one will just be arbitrarily chosen as the value for that key, is that correct?
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.
I have now added a comment to this (renamed) function. Is this rationale OK? Maybe there's a better way:
-- | We want to identify all of the unique (terrain, entity facade) pairs.
-- However, "EntityFacade" includes a "Display" record, which contains more
-- fields than desirable for use as a unique key.
-- Therefore, we extract just the entity name for use in a
-- (terrain, entity name) key, and couple it with the original
-- (terrain, entity facade) pair in a Map.
"Stub" maps generated by the World Editor (#873) do not define any robots. It is convenient to view these via: scripts/play.sh --scenario stub.yaml --cheat However, the static needs to be suppressed in creative mode when robots do not exist.
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.
🚀
Towards #558
I was motivated to build this after finding that editing scenario maps directly in the YAML file is rather constraining.
What I've implemented so far
--cheat
modeuiWorldEditor
field ofUIState
record.Controller.hs
andView.hs
to extract functions utilized by the World Editor (towards Split large Haskell modules #707)Vision
Demos
Round-trip with random world
Then Ctrl+e, tab down to the Save button, hit Enter to save the map
In another tab run:
Toggle between tabs to compare, observe the derived map is an identical 41x21 subset.