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

Iron #373

Merged
merged 31 commits into from
Jun 14, 2022
Merged

Iron #373

merged 31 commits into from
Jun 14, 2022

Conversation

xsebek
Copy link
Member

@xsebek xsebek commented Jun 8, 2022

  • add iron ore, iron mine and iron vein (closes Add iron to allow more advanced recipes #93)
    • split gear into iron/wooden gear
    • add metal drill
    • add faster recipes with the metal drill
  • add compass (closes Compass device #341)
  • handle multiple entities providing the same capability
    • try to find if the robot has at least one entity providing the capability
    • when no entity could provide the capability rejects it too
  • list required devices in the Incapable error (closes Suggest required entities #342)

@xsebek xsebek requested a review from byorgey June 8, 2022 22:15
@restyled-io restyled-io bot mentioned this pull request Jun 8, 2022
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

Some initial comments. I haven't finished looking carefully through the changes regarding how we check for required devices etc.

data/entities.yaml Outdated Show resolved Hide resolved
data/entities.yaml Outdated Show resolved Hide resolved
data/entities.yaml Outdated Show resolved Hide resolved
data/entities.yaml Outdated Show resolved Hide resolved
data/entities.yaml Show resolved Hide resolved
src/Swarm/Game/Step.hs Show resolved Hide resolved
@xsebek
Copy link
Member Author

xsebek commented Jun 9, 2022

Thanks for taking a look @byorgey, the copy-pasting is definitely a problem 😅

I spent most type debugging the capability checking, so now that it (hopefully?) works I will come back and streamline it.

@byorgey
Copy link
Member

byorgey commented Jun 9, 2022

By the way, I very much like all the new recipes, the way metal drill does things faster than a regular drill, etc.

xsebek added 4 commits June 10, 2022 13:47
- add iron ore
- add iron mine and iron vein
- section the recipes for easier discoverability
- split gear into iron/wooden gear
- add drill test challenge!
- add recipes with metal drill
- make iron veins slightly more likely
xsebek and others added 2 commits June 10, 2022 13:54
- try to find if robot has at least one
  entity providing capability
- when no entity could provide
  the capability reject it too

This "improvement" only concerns
build and reprogram comands.
They could use a refactor after this,
as the common capability checking
part should be moved into helper
function or to Entity.hs
Co-authored-by: Brent Yorgey <byorgey@gmail.com>
@xsebek
Copy link
Member Author

xsebek commented Jun 10, 2022

That Restyled action is a bit annoying since I by mistake pushed to my repo which confused Restyled and it wants to merge to main. 🤦

src/Swarm/Game/Step.hs Outdated Show resolved Hide resolved
xsebek added 4 commits June 10, 2022 23:23
- refactor out required device checking to common function
- add a fun little linguistic function :)
There are a few reasons for this:
- we do not have direction map entry for it (uses default)
- it makes it easier to check
- like forward is (1*dir) and back (-dir) so down is (0*dir)

Really it is the weirdest direction because once you turn down
you can not look up again without compass.
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, but there's still something I don't understand going on with drilling in creative mode.

data/entities.yaml Outdated Show resolved Hide resolved
src/Swarm/Game/Entity.hs Outdated Show resolved Hide resolved
src/Swarm/Game/Entity.hs Show resolved Hide resolved
src/Swarm/Game/Scenario.hs Show resolved Hide resolved
src/Swarm/Game/Step.hs Show resolved Hide resolved
src/Swarm/Util/Yaml.hs Outdated Show resolved Hide resolved
)
-- check that there are in fact devices to provide every required capability
(creative || not (any null deviceSets))
`holdsOrFail` [singularSubjectVerb subject "can", "not perform an impossible task"]
Copy link
Member

Choose a reason for hiding this comment

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

To make sure I'm understanding correctly, if this "impossible task" error is triggered, it means that there do not exist any devices to provide the required capability, right? As in, we have some capability but no actual devices yet in entities.yaml which provide it?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I actually got this error while playing in classic mode and it's very mysterious. How hard would it be to output the name of the capability for which there is no device, with a message apologizing and a link to the issue tracker?

Copy link
Member Author

@xsebek xsebek Jun 12, 2022

Choose a reason for hiding this comment

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

I expected this to only come up for CGod tier tasks, like create "life", where the message would make sense.

Maybe I could check if it is CGod and otherwise link to the issue. Something like:

[ singularSubjectVerb subject "do", "not have", indefiniteQ capName
, "capability, because no device provides it."
, "\nSee https://github.com/swarm-game/swarm/issues/26."
]

However, I feel like it should be Incapable exceptions job to figure this out. EDIT: Actually Incapable is a different situation, but I wish I could use some common code for it here.

Copy link
Member Author

@xsebek xsebek Jun 12, 2022

Choose a reason for hiding this comment

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

Some of it was common, but building and reprogramming can give a more specialised error message, so I keep some duplicate code. Oh well, at least I get to close #342 😇

Copy link
Member

Choose a reason for hiding this comment

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

Also, just an update: the reason I got this error while playing in classic mode was actually due to #389 . So that was making it more mysterious than necessary.

src/Swarm/Language/Syntax.hs Show resolved Hide resolved
@restyled-io restyled-io bot mentioned this pull request Jun 12, 2022
@xsebek xsebek marked this pull request as ready for review June 12, 2022 14:16
@xsebek
Copy link
Member Author

xsebek commented Jun 12, 2022

Everything looks good to me, but there's still something I don't understand going on with drilling in creative mode.

@byorgey it was actually more than drilling, by mistake I did not install any required entities if they were not present in inventory.

The commit 9e24293 should fix that 🙂

formatIncapable em f caps tm
| CGod `S.member` caps =
unlinesExText
[ "Can not perform an impossible task:"
Copy link
Member

Choose a reason for hiding this comment

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

I think this error message could still be improved. I don't think it is going to make much sense if a player stumbles across it. e.g. imagine that they type as accidentally and notice that it has a type, then they wonder what it does so they try it out. They are going to wonder why the task is impossible. Somehow we should convey that this is a command that is not available during normal play. We can definitely make it funny though. Some ideas:

  • "This command is not available to mere mortals" / "This command is forbidden"
  • "Thou shalt not utter such blasphemy"
  • If we wanted to be more informative and less funny, "This command is only available to system robots and scenario definitions".

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the Elden ring is now popular, so blasphemy it is:

incapableError (S.singleton CGod) (TConst As)
-- Thee shalt not utter such blasphemy:
--   'as'
--   If't be true thee wanteth to playeth god, then tryeth Creative game.

@byorgey
Copy link
Member

byorgey commented Jun 12, 2022

This is looking great! 🚀

@byorgey
Copy link
Member

byorgey commented Jun 13, 2022

So I ran into one additional issue while testing out some things. In creative mode I tried something like this:

def forever = \c. c ; forever c end
def unblock = try {drill forward} {} end
def push = unblock; move end

build {forever (push; build {turn right; forever push})}

The intention is to make a robot that moves in a straight line forever, drilling through obstacles along the way, and at each step creating a robot that does the same in a perpendicular direction. This should be a very quick way to drill through all the mountains in a particular quadrant. Obviously this would be very expensive to do in classic mode, but I think it should work in creative mode.

However, it doesn't work. The initial robot works fine, drilling along and building another robot at each step. However, all those secondary robots don't end up with a drill device, and crash with a fatal error when they try to execute the drill command.

@byorgey
Copy link
Member

byorgey commented Jun 13, 2022

But up to you whether we want to go ahead and merge this and file a separate issue re the problems I mentioned above! At this point I would actually lean towards doing it that way rather than hold this up longer.

@byorgey
Copy link
Member

byorgey commented Jun 13, 2022

A capability error reporting bug that I am likewise happy to split out into a separate issue after merging this, just want to write it down while it's fresh in my head:

  • Start a classic mode game with --cheat
  • Switch into creative mode and create "branch predictor"
  • Switch back out of creative mode
  • Type build {move; if (1 > 2) {} {}}
    • As expected we get an error saying no device provides the compare capability.
  • Now type build {move; turn left; if (1 > 2) {} {}}
    • All we have done is add turn left; we would expect to get the same error. Instead we get an error like You do not have the devices required for: 'move; turn left; if (1 > 2) {noop} {noop}' please obtain: - branch predictor
    • But we do have a branch predictor, so this error is bogus. Not sure yet whether the capability checking or error reporting is to blame.

@xsebek
Copy link
Member Author

xsebek commented Jun 14, 2022

@byorgey I think your first bug is present on main too, the Drill capability is not reported by requiredCaps.
The difference is, that I call that a Fatal error because build should have given it to the robot, while main drives the robots to rocks and there they stop without a drill.

See the debug logs I added:

Main

Screenshot from 2022-06-14 17-01-12

Iron

I was also missing standard devices, so I added them back:
Screenshot from 2022-06-14 16-48-55
Screenshot from 2022-06-14 17-19-14


I have no idea what the second bug is, but I hope it is not really my fault 😄

@xsebek xsebek added the merge me Trigger the merge process of the Pull request. label Jun 14, 2022
@mergify mergify bot merged commit bda16b7 into swarm-game:main Jun 14, 2022
xsebek added a commit that referenced this pull request Jun 22, 2022
Fixes the mistake introduces in #373.
mergify bot pushed a commit that referenced this pull request Jun 22, 2022
- fix the mistake introduces in #373
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.

Suggest required entities Compass device Add iron to allow more advanced recipes
3 participants