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.

Sign up for GitHub

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

Jump to bottom

implement Equip/Unequip #887

Merged
merged 1 commit into from
Dec 14, 2022
Merged

implement Equip/Unequip #887

merged 1 commit into from
Dec 14, 2022

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Dec 7, 2022

See comment on #883

kostmo
kostmo commented Dec 7, 2022
View reviewed changes
src/Swarm/Game/Step.hs Outdated
@@ -1488,6 +1499,16 @@ execConst c vs s k = do
let msg = "The operator '$' should only be a syntactic sugar and removed in elaboration:\n"
in throwError . Fatal $ msg <> badConstMsg
where
installSelf item focusedID myID = do
Copy link
Member Author

Choose a reason for hiding this comment

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

This implementation is now shared between install and equip.

kostmo
kostmo commented Dec 7, 2022
View reviewed changes
src/Swarm/Language/Syntax.hs
@@ -542,6 +546,8 @@ constInfo c = case c of
["The current location has to be empty for this to work."]
Give -> command 2 short "Give an item to another robot nearby."
Install -> command 2 short "Install a device from inventory on a robot."
Copy link
Member Author

Choose a reason for hiding this comment

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

It will be quite a bit more work to remove the Install command. Let's leave it around but consider it "deprecated".

Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to change the install tutorial to an equip tutorial? Even in the tutorial, it tells you to install something on yourself...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but let's address that in #907.

@kostmo
Copy link
Member Author

kostmo commented Dec 7, 2022

Tested with stack run -- --scenario classic to exercise both equip and unequip, using compass and turn west. After unequipping the compass, it doesn't let me turn by cardinal directions anymore.

@kostmo kostmo requested a review from byorgey December 7, 2022 07:28
@kostmo kostmo force-pushed the equip-unequip branch 4 times, most recently from 93f7d82 to e0522d3 Compare December 7, 2022 20:43
@restyled-io restyled-io bot mentioned this pull request Dec 7, 2022
Closed
@kostmo kostmo force-pushed the equip-unequip branch 3 times, most recently from b9c23e4 to 0e96974 Compare December 8, 2022 04:16
@kostmo kostmo marked this pull request as ready for review December 9, 2022 06:42
@kostmo kostmo added the L-Commands Built-in commands (e.g. move, try, if, ...) in the Swarm language. label Dec 10, 2022
byorgey
byorgey reviewed Dec 13, 2022
View reviewed changes
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.

This is looking good. We also need to update the help text for an appropriate device to mention equip and unequip. Since you put these commands under CGive, that device would be grabber. However, the poor grabber is already very overloaded with providing four different commands. It's also unclear why a grabber would be needed to equip and unequip on yourself (I imagine the grabber arm doing stuff outside the robot). We might consider (1) creating a new capability, CEquip, which is (2) provided by a new device.

@kostmo
Copy link
Member Author

kostmo commented Dec 13, 2022

Since you put these commands under CGive

It's actually using the CInstall capability, not CGive.

@restyled-io restyled-io bot mentioned this pull request Dec 13, 2022
Closed
@byorgey
Copy link
Member

byorgey commented Dec 13, 2022

Since you put these commands under CGive

It's actually using the CInstall capability, not CGive.

Oh, ok, but that capability is still provided by a grabber.

@kostmo kostmo mentioned this pull request Dec 14, 2022
Closed
@kostmo
Copy link
Member Author

kostmo commented Dec 14, 2022

We might consider (1) creating a new capability, CEquip, which is (2) provided by a new device.

How about a device called a "welder" that provides CEquip? Let's use it as a placeholder and change it later if we like. Though I think it has a couple good points going for it:

  • conveys a proper sense of semi-permanence/solidity to the "attachment" of items that one equips
  • assuming robots are metal, it is applicable regardless of the "material" of the thing being equipped, with metal brackets etc. being implicit in the attachment process.

@byorgey
Copy link
Member

byorgey commented Dec 14, 2022

How about a device called a "welder" that provides CEquip?

I like it! It's kinda like the plasma cutter we used to have for salvage, but less destructive. 😄

@kostmo kostmo force-pushed the equip-unequip branch 2 times, most recently from 5dd3fde to f4be72e Compare December 14, 2022 04:43
byorgey
byorgey approved these changes Dec 14, 2022
View reviewed changes
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.

Looks good! Eventually we'll want a recipe for welder.

@kostmo kostmo added the merge me Trigger the merge process of the Pull request. label Dec 14, 2022
@restyled-io restyled-io bot mentioned this pull request Dec 14, 2022
Closed
@mergify mergify bot merged commit e07b2c7 into main Dec 14, 2022
@kostmo kostmo deleted the equip-unequip branch December 14, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byorgey byorgey byorgey approved these changes

Assignees
No one assigned
Labels
L-Commands Built-in commands (e.g. move, try, if, ...) in the Swarm language. merge me Trigger the merge process of the Pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kostmo @byorgey