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

Use cmap_to_glyph(S_stone) as "unset" glyph, not 0 (which is also PM_GIANT_ANT). #179

Merged
merged 2 commits into from
Jun 23, 2021
Merged

Use cmap_to_glyph(S_stone) as "unset" glyph, not 0 (which is also PM_GIANT_ANT). #179

merged 2 commits into from
Jun 23, 2021

Conversation

heiner
Copy link
Contributor

@heiner heiner commented Jun 23, 2021

No description provided.

@heiner heiner requested a review from cdmatters June 23, 2021 13:29
@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 Jun 23, 2021
@heiner heiner requested a review from samvelyan June 23, 2021 13:30
samvelyan
samvelyan reviewed Jun 23, 2021
View reviewed changes
Copy link
Contributor

@samvelyan samvelyan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me. I've updated the tileset functionality accordingly. Specifically, when mapping glyph_ids to tile_ids for rendering, I replace NO_GLYPH with "dark part of the room" glyph id (corresponds to black square tile).

@heiner
Copy link
Contributor Author

heiner commented Jun 23, 2021

Looks good to me. I've updated the tileset functionality accordingly. Specifically, when mapping glyph_ids to tile_ids for rendering, I replace NO_GLYPH with "dark part of the room" glyph id (corresponds to black square tile).

That's a good point. Looking at https://github.com/facebookresearch/nle/blob/master/src/display.c#L1587 that is what NetHack does itself, so we should rather go with that.

I updated this PR.

@heiner heiner changed the title Use NO_GLYPH as "unset" glyph, not 0 (which is also PM_GIANT_ANT). Use cmap_to_glyph(S_stone) as "unset" glyph, not 0 (which is also PM_GIANT_ANT). Jun 23, 2021
cdmatters
cdmatters approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@cdmatters cdmatters left a comment

Choose a reason for hiding this comment

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

lgtm

@heiner heiner merged commit dd7241c into master Jun 23, 2021
@heiner heiner deleted the heiner/usenoglyph branch June 23, 2021 17:28
@heiner heiner mentioned this pull request Oct 8, 2021
Closed
heiner pushed a commit that referenced this pull request Oct 8, 2021
Fix NetHackScout environment following #179.
48fb87b 
Fixes #275.
heiner pushed a commit that referenced this pull request Oct 8, 2021
Fix NetHackScout environment following #179.
9414619 
Fixes #275.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@samvelyan samvelyan samvelyan left review comments

@cdmatters cdmatters cdmatters approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@heiner @cdmatters @samvelyan @facebook-github-bot