Add menu_strs observations. #207

cdmatters · 2021-07-12T17:08:37Z

This is an observation that renders windows that are menus that pop up.

This is a DRAFT to discuss the issues that may be present in #188. A proposed solution could look like like this draft.

NetHack has a window type called NHW_MENU that is used to render small items of text and multichoice menus (both of them). Example contexts are when casting spells, choosing what to drop, what to pick up and put down when opening chests, and also just descrtiptions of what is in the floor.

There are two types of menu windows: ones that have menu_items - which include a letter and may be 'selected' (like casting spells, picking up items etc); and onethats have strings - which are just small items of text (like descriptions of what is in a place).

I propose adding a new observation, with the capacity of extracting all the useful data from the menu_items, and then perhaps placing a flag in misc to indicated first - are we in a menu - and second - what type of menu is this?

Potential problems:

How many menus can you have simultaneously? Is one the max?
Are there any use cases that this misses? This doesnt cover large sections of text, for instance.
How does this affect performance?

I'm sure there are other questions, but this is a start.

Note: There is onle line where I readjust the size of the vectors of windows. For some reason we don't 'delete' a window when it is deleted, we just manually tombstone with null, which affects size() function on the vector. I assume theres a very good reason for this, since it seems a little counter intuitive, (we only do the resize when creating a new_window?). I have done the resize to get the code to work, but i would suggest just removing and adding items as usual if we can get there...

cdmatters · 2021-07-12T17:19:09Z

Wow really fantastic - already a segfault SMH

cdmatters · 2021-07-12T17:19:42Z

I tested this manually, but will dig into it tomorrow

paulkent-um · 2021-07-12T17:49:12Z

Hey, OP of #188 here.

Are there any use cases that this misses? This doesnt cover large sections of text, for instance.

As a matter of fact, there is at least one use case I can immediately think of that this misses.

Since the agent doesn't choose for itself what race, role, and alignment to play, it's kind of important that the agent be able to figure out what was chosen for it, since each of those has substantial gameplay implications. The most obvious way to figure this out is through the "hello Agent and welcome to Nethack" message at the beginning of the game, but that text doesn't appear on the full moon or new moon (since the text noting the phase of the moon replaces it), so that's not a reliable solution. Perhaps the agent could figure out role by reading off the "agent the (rank)" straight from ttychars, or by reviewing their starting equipment, but race and alignment don't always have those tells. The only surefire way I know of to find out your race and alignment at game start is with the Attributes action (action 25, keypress 24)... which displays as a large section of text but not a menu.

This is an observation that renders windows that are menus that pop up.

cdmatters · 2021-07-15T15:33:39Z

Hi @paulkent-um. Thanks for chiming in! 😄

Before moving on, let me just refocus of this point of this PR - to serialize pop-up windows rather than anything else. While I appreciate that determining the race and role of your player might be useful to you, if you are looking for a more direct route in to getting this info you should open a different issue and we can discuss there.

As it happens the output of the attributes command (Ctrl + X) is technically pop-up menu (NHW_MENU) of the string type mentioned above, and actually would be covered by this PR (I have tested it). However, here we uncover our first big problem.

The NetHack game creates the window object, but it is up to implementation of the windowing system to decide how to display it if it is too big. This is not a problem for spells and items on the floor, but for attributes it is always too many rows for the screen and gets paginated.

In the case of this pagination, the current draft here would always only return the first half of the window text in the numpy array, even when the screen is displaying the second half of the page. This is clearly a bug, but hacks around this with some local pagination on the winrl.cc end seem an invitation to even more bugs. Thoughts from @heiner welcome at this point.

cdmatters · 2021-07-15T15:37:33Z

Wow really fantastic - already a segfault SMH

BTW this segfault came from the bug mentioned above: (number of rows in the window > rows in the screen) -> SEGV

cdmatters · 2021-07-15T16:06:26Z

Here are the profiling results.

Note, this nethack environment (NetHack-v0) isnt really doing anything that involves windows, but the copying will still be be taking place

heiner

I'll need to dig into this more to say something more useful here.

heiner · 2021-07-16T10:51:37Z

win/rl/winrl.cc

@@ -290,6 +290,13 @@ NetHackRL::fill_obs(nle_obs *obs)
        obs->misc[0] = in_yn_function;
        obs->misc[1] = in_getlin;
        obs->misc[2] = xwaitingforspace;
+        obs->misc[3] = false;


Be careful with C++ booleans like true/false vs C bools. They can in some situations not be the same thing and map to objects that evaluate to different logical values (!)

In this case it's ok because the lhs is an int, so the type will just get casted. Given that, it might make sense to write an int here in the first place.

heiner · 2021-07-16T10:53:20Z

win/rl/winrl.cc

+                // to be available, and this one goes into menu_strs
+                found_menu = true;
+                rl_window *win = windows_[i].get();
+                assert(!win->menu_items.empty() ^ !win->strings.empty());


Note that asserts are stripped out of non-debug builds.

heiner · 2021-07-16T10:54:50Z

win/rl/winrl.cc

+    if (obs->menu_strs) {
+        bool found_menu = false;
+        for (int i = 0; i < windows_.size(); ++i) {
+            if (i != WIN_INVEN && windows_[i].get()->type == NHW_MENU) {


I'll have to very carefully check the logic here. I'm especially surprised there's no break statement here?

Will get back to this.

Yes - I'm not sure what to do if there are two windows - so I didn't put a break while testing to see if I could trigger it. If we go with a solution like this we should have a break.

heiner · 2021-07-16T10:56:11Z

win/rl/winrl.cc

@@ -670,6 +721,7 @@ NetHackRL::destroy_nhwindow_method(winid wid)
 {
    DEBUG_API("rl_destroy_nhwindow(wid=" << wid << ")" << std::endl);
    windows_[wid].reset(nullptr);
+    windows_.resize(wid);


This seems strange?

(1) It renders the line before needless.
(2) It assumes no index i >= wid is currently used?

Yes for some reason I thought the windows were stored in a stack-style data structure (largely verified in practice) but maybe this is all wrong. Either way should probably go read more code.

cdmatters · 2021-07-15T15:38:44Z

win/rl/winrl.cc

@@ -670,6 +721,7 @@ NetHackRL::destroy_nhwindow_method(winid wid)
 {
    DEBUG_API("rl_destroy_nhwindow(wid=" << wid << ")" << std::endl);
    windows_[wid].reset(nullptr);


We could just remove this and save the reset resize

cdmatters · 2021-07-19T10:06:09Z

win/rl/winrl.cc

@@ -670,6 +721,7 @@ NetHackRL::destroy_nhwindow_method(winid wid)
 {
    DEBUG_API("rl_destroy_nhwindow(wid=" << wid << ")" << std::endl);
    windows_[wid].reset(nullptr);
+    windows_.resize(wid);


Yes for some reason I thought the windows were stored in a stack-style data structure (largely verified in practice) but maybe this is all wrong. Either way should probably go read more code.

cdmatters · 2021-07-19T10:07:24Z

win/rl/winrl.cc

+    if (obs->menu_strs) {
+        bool found_menu = false;
+        for (int i = 0; i < windows_.size(); ++i) {
+            if (i != WIN_INVEN && windows_[i].get()->type == NHW_MENU) {


Yes - I'm not sure what to do if there are two windows - so I didn't put a break while testing to see if I could trigger it. If we go with a solution like this we should have a break.

paulkent-um · 2021-07-22T03:17:33Z

Hey, can I follow up on this? Do you know when/if I can expect this to be merged into the main branch?

facebook-github-bot · 2023-11-18T08:07:30Z

Hi @condnsdmatters!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

cdmatters requested a review from heiner July 12, 2021 17:08

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 Jul 12, 2021

cdmatters mentioned this pull request Jul 12, 2021

Include "Pop-up" windows in observation #188

Open

cdmatters force-pushed the eric/menu-window branch 3 times, most recently from 10905e2 to eb2f48d Compare July 12, 2021 17:29

cdmatters force-pushed the eric/menu-window branch 2 times, most recently from 75818d1 to 4f76029 Compare July 15, 2021 15:03

Add menu_strs observations.

ed68e16

This is an observation that renders windows that are menus that pop up.

cdmatters force-pushed the eric/menu-window branch from 4f76029 to ed68e16 Compare July 15, 2021 15:12

Fixup profiling to include new observations.

6c85215

heiner reviewed Jul 16, 2021

View reviewed changes

cdmatters commented Jul 19, 2021

View reviewed changes

adyomin mentioned this pull request Aug 17, 2021

How can I get current character's rol-rac-ali-gen combo for NetHackChallenge task? #233

Closed

heiner mentioned this pull request Oct 12, 2021

Ideas for NLE, Fall/Winter 2021 edition #278

Open

apowers313 mentioned this pull request Aug 13, 2024

TODOs 2024 heiner/nle#6

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add menu_strs observations. #207

Add menu_strs observations. #207

cdmatters commented Jul 12, 2021

cdmatters commented Jul 12, 2021

cdmatters commented Jul 12, 2021

paulkent-um commented Jul 12, 2021 •

edited

Loading

cdmatters commented Jul 15, 2021 •

edited

Loading

cdmatters commented Jul 15, 2021

cdmatters commented Jul 15, 2021

heiner left a comment

heiner Jul 16, 2021

heiner Jul 16, 2021

heiner Jul 16, 2021

cdmatters Jul 19, 2021

heiner Jul 16, 2021

cdmatters Jul 19, 2021

cdmatters Jul 15, 2021

cdmatters Jul 19, 2021

cdmatters Jul 19, 2021

paulkent-um commented Jul 22, 2021

facebook-github-bot commented Nov 18, 2023

Add menu_strs observations. #207

Are you sure you want to change the base?

Add menu_strs observations. #207

Conversation

cdmatters commented Jul 12, 2021

cdmatters commented Jul 12, 2021

cdmatters commented Jul 12, 2021

paulkent-um commented Jul 12, 2021 • edited Loading

cdmatters commented Jul 15, 2021 • edited Loading

cdmatters commented Jul 15, 2021

cdmatters commented Jul 15, 2021

heiner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulkent-um commented Jul 22, 2021

facebook-github-bot commented Nov 18, 2023

Process

paulkent-um commented Jul 12, 2021 •

edited

Loading

cdmatters commented Jul 15, 2021 •

edited

Loading