Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Fix bug in winrl getch which makes messages disappear forever #143

Merged
merged 1 commit into from
Dec 14, 2020

Conversation

tscmoo
Copy link
Contributor

@tscmoo tscmoo commented Dec 13, 2020

This method replaces tty_ngetch, and does not call it as per the comment, but it leaves out some important code (ref https://github.com/facebookresearch/nle/blob/master/win/tty/wintty.c#L3486-L3538). This PR adds this code to the winrl getch.

The bug is that if you press escape on a --more-- prompt, WIN_STOP would be set (as a signal to escape from the more prompt and skip any proceeding messages), and this flag would be reset in tty_getch. Without this reset, nle proceeds to skip every message in the entire game from that point on, with no recovery (this means no messages are displayed, not even "it's a wall" etc).

the toplin code is part of a tty state machine in topl.c related to multi-line messages and more prompts, and while I can't entirely figure out the consequence of removing it, I think it's safer to keep it here.

This method replaces tty_ngetch, and does not call it as per the comment, but it leaves out some important code (ref https://github.com/facebookresearch/nle/blob/master/win/tty/wintty.c#L3486-L3538). This PR adds this code to the winrl getch.

The bug is that if you press escape on a --more-- prompt, WIN_STOP would be set (as a signal to escape from the more prompt and skip any proceeding messages), and this flag would be reset in tty_getch. Without this reset, nle proceeds to skip *every* message in the entire game from that point on, with no recovery.

the toplin code is part of a tty state machine  in topl.c related to multi-line messages and more prompts, and while I can't entirely figure out the consequence of removing it, I think it's safer to keep it here.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 13, 2020
@heiner heiner self-requested a review December 14, 2020 12:21
Copy link
Contributor

@heiner heiner left a comment

Choose a reason for hiding this comment

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

Thanks!

This one is one me. And one more indication that perhaps training on the tty output itself would be best.

@tscmoo tscmoo merged commit 3a4915b into master Dec 14, 2020
@tscmoo tscmoo deleted the moo/fix-winrl-getch-messages branch December 14, 2020 14:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants