search

UMDLARS / one_night_in_sf

A story game demonstrating why physical security is important and complex!
1 stars 2 forks source link

Bug fixes / QoL improvements #227

Closed libraun closed 3 months ago

libraun commented 4 months ago

Fixes:

Other Additions:

pahp commented 4 months ago

Hi @libraun --

Thanks for this work! I did some testing, and your notes gave me an idea...

> remove pins
No need to concern yourself with that.

I'm not sure that this is better than That wouldn't be useful. ...?

Seems like maybe in addition to remove (hinges|pins) with flathead it should say something if you type remove (hinges|pins) -- like Remove them with what? -- is that straightforward to do, @spacehobo ?

pahp commented 4 months ago

Another thing @libraun -- I think the emergency exit door should be present (and thanks for making it!), but when you try to go through it, it tells you to wait until you've completed things. As implemented, it lets you into a new room (the top of the stairs?), but that room goes nowhere... it breaks disbelief; why can't I just try to go downstairs... OMG where are the stairs?

So again, I think that room should be inaccessible until the tasks are complete, and then once you are let through there is a fire escape that works however we intend for it to work. Thanks!

pahp commented 3 months ago

@libraun -- See comment on PR. I don't think you should be able to go into the fire escape room until you have all the items. It is weird to go into a room that has no visible exits. Also, presumably the fire exit would have stairs, but they're not visible (because we don't want them to use them).

It would be cleaner to simply stop them from going through the fire exit door unless they have all the items.

I also think that the game should not be in a broken state, so I think that once the player has all the items, if they try to use the door, it just directs them to the elevator for now ("Do you really want to carry an IBM 5100 down 13 flights of stairs?"). We can add in some kind of funny fire escape thing later if we want.

So...

  1. If they try to use the fire escape door, it should say no if they do not have all the items.
  2. If they do have all the items, it should tell them to use the elevator in a funny way.

Thanks!

libraun commented 3 months ago

Thanks for these notes!! Per the fire escape: I actually caught that right before you emailed me this; I thought it was weird that the player opens the door (to “inspect” the fire escape) without closing it again, so I made an edit which clarifies that. Additionally there is a check in there that switches up the dialogue, if the player has MAX_SCORE.

I’ll go through these more at home, and get up to date. Thanks again!

On Wed, Mar 6, 2024 at 11:11 AM Peter A. H. Peterson < @.***> wrote:

Hi @libraun https://github.com/libraun --

Thanks for this work! I did some testing, and your notes gave me an idea...

remove pins No need to concern yourself with that.

I'm not sure that this is better than That wouldn't be useful. ...?

Seems like maybe in addition to remove (hinges|pins) with flathead it should say something if you type remove (hinges|pins) -- like Remove them with what? -- is that straightforward to do, @spacehobo https://github.com/spacehobo ?

— Reply to this email directly, view it on GitHub https://github.com/UMDLARS/one_night_in_sf/pull/227#issuecomment-1981378161, or unsubscribe https://github.com/notifications/unsubscribe-auth/APBM67MMUAZF32SLC33Z6BDYW5E25AVCNFSM6AAAAABDSBBMH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBRGM3TQMJWGE . You are receiving this because you were mentioned.Message ID: @.***>

libraun commented 3 months ago

I was also just looking at how extension works in PunyInform, so I’m still curious about that last one.

On Thu, Mar 7, 2024 at 3:58 PM Jet Braun @.***> wrote:

Thanks for these notes!! Per the fire escape: I actually caught that right before you emailed me this; I thought it was weird that the player opens the door (to “inspect” the fire escape) without closing it again, so I made an edit which clarifies that. Additionally there is a check in there that switches up the dialogue, if the player has MAX_SCORE.

I’ll go through these more at home, and get up to date. Thanks again!

On Wed, Mar 6, 2024 at 11:11 AM Peter A. H. Peterson < @.***> wrote:

Hi @libraun https://github.com/libraun --

Thanks for this work! I did some testing, and your notes gave me an idea...

remove pins No need to concern yourself with that.

I'm not sure that this is better than That wouldn't be useful. ...?

Seems like maybe in addition to remove (hinges|pins) with flathead it should say something if you type remove (hinges|pins) -- like Remove them with what? -- is that straightforward to do, @spacehobo https://github.com/spacehobo ?

— Reply to this email directly, view it on GitHub https://github.com/UMDLARS/one_night_in_sf/pull/227#issuecomment-1981378161, or unsubscribe https://github.com/notifications/unsubscribe-auth/APBM67MMUAZF32SLC33Z6BDYW5E25AVCNFSM6AAAAABDSBBMH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBRGM3TQMJWGE . You are receiving this because you were mentioned.Message ID: @.***>

spacehobo commented 3 months ago

This is kind of a huge pull request that lumps a lot of things together, so it's going to be difficult to review. It's really best practice to keep commits and PRs small, and maybe make one per issue where possible.

The first thing I'm not sure about is the binding of remove to ThrowAt, but in a pattern that doesn't have any indirect objects. At best this needs comments, but at worst it seems like it may be the result of a misunderstanding. Can you help me understand why that's there?

Second, I don't see any tests for any of the added functionality. We need to be able to just run make to verify that it does what you think it does! The fire escape absolutely needs tests before we can merge this.

Third, the hinge puzzle is either getting complex enough that it needs an actual door object, or it needs more logic to display things differently once the pins are removed.

libraun commented 3 months ago

Oops, that “remove” binding shouldn’t have made it in there. Sorry for all the confusion, I was worried I might’ve made this one way too big. I’ll go through and try to make a log of all the changes for this PR. I’ll try to make it as complete as possible, but a lot of the changes were just typo fixes, and some rewordings to make things more apparent where needed.

I don’t suppose there’s a way to “split” pull requests into commits?

On Thu, Mar 7, 2024 at 5:20 PM Abolish ICE @.***> wrote:

This is kind of a huge pull request that lumps a lot of things together, so it's going to be difficult to review. It's really best practice to keep commits and PRs small, and maybe make one per issue where possible.

The first thing I'm not sure about is the binding of remove to ThrowAt, but in a pattern that doesn't have any indirect objects. At best this needs comments, but at worst it seems like it may be the result of a misunderstanding. Can you help me understand why that's there?

Second, I don't see any tests for any of the added functionality. We need to be able to just run make to verify that it does what you think it does! The fire escape absolutely needs tests before we can merge this.

Third, the hinge puzzle is either getting complex enough that it needs an actual door object, or it needs more logic to display things differently once the pins are removed.

— Reply to this email directly, view it on GitHub https://github.com/UMDLARS/one_night_in_sf/pull/227#issuecomment-1984767608, or unsubscribe https://github.com/notifications/unsubscribe-auth/APBM67MHFQJNNCK2IIFQWQTYXDY5NAVCNFSM6AAAAABDSBBMH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBUG43DONRQHA . You are receiving this because you were mentioned.Message ID: @.***>

libraun commented 3 months ago

Essentially, the “remove” binding was just to test and see if items could be removed from the player’s inventory. It didn’t work out, so disregard that portion.

I’ll also add a good amount of comments, hopefully that should make it easier to disambiguate.

On Thu, Mar 7, 2024 at 5:37 PM Jet Braun @.***> wrote:

Oops, that “remove” binding shouldn’t have made it in there. Sorry for all the confusion, I was worried I might’ve made this one way too big. I’ll go through and try to make a log of all the changes for this PR. I’ll try to make it as complete as possible, but a lot of the changes were just typo fixes, and some rewordings to make things more apparent where needed.

I don’t suppose there’s a way to “split” pull requests into commits?

On Thu, Mar 7, 2024 at 5:20 PM Abolish ICE @.***> wrote:

This is kind of a huge pull request that lumps a lot of things together, so it's going to be difficult to review. It's really best practice to keep commits and PRs small, and maybe make one per issue where possible.

The first thing I'm not sure about is the binding of remove to ThrowAt, but in a pattern that doesn't have any indirect objects. At best this needs comments, but at worst it seems like it may be the result of a misunderstanding. Can you help me understand why that's there?

Second, I don't see any tests for any of the added functionality. We need to be able to just run make to verify that it does what you think it does! The fire escape absolutely needs tests before we can merge this.

Third, the hinge puzzle is either getting complex enough that it needs an actual door object, or it needs more logic to display things differently once the pins are removed.

— Reply to this email directly, view it on GitHub https://github.com/UMDLARS/one_night_in_sf/pull/227#issuecomment-1984767608, or unsubscribe https://github.com/notifications/unsubscribe-auth/APBM67MHFQJNNCK2IIFQWQTYXDY5NAVCNFSM6AAAAABDSBBMH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBUG43DONRQHA . You are receiving this because you were mentioned.Message ID: @.***>

spacehobo commented 3 months ago

I don’t suppose there’s a way to “split” pull requests into commits?

Mainly the way to do it I think is to make new branches and cherry-pick commits into those branches, and propose each one separately. I tend to name my branches after issue numbers, and begin by adding a failing test for that issue.

libraun commented 3 months ago

That’s smart, I’ll see what I can do there at this point in terms of moving commits around (and maybe splicing them into different PR’s if possible). Definitely noted for the future.

On Thu, Mar 7, 2024 at 6:03 PM Abolish ICE @.***> wrote:

I don’t suppose there’s a way to “split” pull requests into commits?

Mainly the way to do it I think is to make new branches and cherry-pick commits into those branches, and propose each one separately. I tend to name my branches after issue numbers, and begin by adding a failing test for that issue.

— Reply to this email directly, view it on GitHub https://github.com/UMDLARS/one_night_in_sf/pull/227#issuecomment-1984810303, or unsubscribe https://github.com/notifications/unsubscribe-auth/APBM67OL6TJA2HUF7CC7D6DYXD56JAVCNFSM6AAAAABDSBBMH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBUHAYTAMZQGM . You are receiving this because you were mentioned.Message ID: @.***>

libraun commented 3 months ago

Another thing @libraun -- I think the emergency exit door should be present (and thanks for making it!), but when you try to go through it, it tells you to wait until you've completed things. As implemented, it lets you into a new room (the top of the stairs?), but that room goes nowhere... it breaks disbelief; why can't I just try to go downstairs... OMG where are the stairs?

So again, I think that room should be inaccessible until the tasks are complete, and then once you are let through there is a fire escape that works however we intend for it to work. Thanks!

Working on this now, should I go ahead and pull these changes into this branch or wait until after the commit? It should only take a couple adjustments to the test file.

Edit: Decided to make the fire escape cheap scenery, and did away with some test cases for it since it's no longer a scene. You can find it in the latest commit for this branch! Do we test cheap scenery too?

libraun commented 3 months ago

Hi @libraun --

Thanks for this work! I did some testing, and your notes gave me an idea...

> remove pins
No need to concern yourself with that.

I'm not sure that this is better than That wouldn't be useful. ...?

Definitely agreed, and fixed! Not sure how that happened.

Seems like maybe in addition to remove (hinges|pins) with flathead it should say something if you type remove (hinges|pins) -- like Remove them with what? -- is that straightforward to do, @spacehobo ?

No luck with this, myself. But I can tell you what doesn't work:

if (second == "" || second == " ") {...}

spacehobo commented 3 months ago

It's looking much better! Remember to test "failures" as well, so that the user tries things that shouldn't work, and you prove that they continue to fail even as we change code.

Also I mentioned on Signal, but you can see the way the include directive can work to let you create a complex game state to use at the start of multiple tests.

libraun commented 3 months ago

It's looking much better! Remember to test "failures" as well, so that the user tries things that shouldn't work, and you prove that they continue to fail even as we change code.

Also I mentioned on Signal, but you can see the way the include directive can work to let you create a complex game state to use at the start of multiple tests.

Wonderful! And testing failures is a great policy that I need to remember to use.

@spacehobo Some tests included! Not many, mostly just tests to make sure the new flathead/tarp change is working, and then one new test for typing "jtitor" twice. Now that I think about it, I'll need some tests for that cat poster too, so expect those soon.

libraun commented 3 months ago

Another thing @libraun -- I think the emergency exit door should be present (and thanks for making it!), but when you try to go through it, it tells you to wait until you've completed things. As implemented, it lets you into a new room (the top of the stairs?), but that room goes nowhere... it breaks disbelief; why can't I just try to go downstairs... OMG where are the stairs?

So again, I think that room should be inaccessible until the tasks are complete, and then once you are let through there is a fire escape that works however we intend for it to work. Thanks!

@pahp It might've been mentioned in Slack, but how is this looking as cheap scenery instead of as it was (a separate room)? Regardless, I understand how you mean by funkiness in terms of opening the door to look into the fire escape. It's disorienting.