Tty rendering for NLE environment

[rockt]

No description provided.

[cdmatters]

Good and clear, some general comments on string addition, and api. Perhaps we could make terminal render mode a sepate 'mode' instead of simply deleting the old code, for users? I will see if we can get the ansi mapping from nethack object etc...

[cdmatters]

in general this string additions can get very slow because in Python string are immutable, and + is a binary operator.
so "some long" + "strings" + "can be" + "expensive" + "to make"

is evaluated as:

("some long" + "strings")  # all copied to "some long strings"
("some long string"+ "can be")  # copied to "some long strings can be"
("some long strings can be"  + "expensive")  ... # etc O(N^2)

Thats why the "Pythonic" thing is to collect the strings in a list and use .join method as you have above, which means the allocation is only done once (O(N))

[cdmatters]

TBF you have done the line.join for for the rows, so N in this case is only like 24 rather than 80, but its still an order of magnitude.

[cdmatters]

can I suggest either

Suggested change

	return self.tty_render(obs, print_guides=False)
	self.tty_render(obs, print_guides=False)
	return

or

Suggested change

	return self.tty_render(obs, print_guides=False)
	image = self.tty_render(obs, print_guides=False)
	print(image)
	return

The current API suggests render doesnt return anything, and so by writing it as either of these examples, we can clearly see that the printing must happen in the tty_render function, and not elsewhere in the code (ie perhaps with the returned value).

in fact I would be tempted to go for the second example, remove the print function from the tty_render method, and rename it as "tty_to_string" or "get_tty_string". That way it can be programatically reused, possibly for logging by downstream users.

[cdmatters]

I think this should work with all ansi compatible terminals, but Im not entirely sure. It could probably be clearer which colour is which as well... But also this might not be a priority.

[rockt]

Thanks @condnsdmatters — PTAL

[cdmatters]

Some nit pick suggestions, but looks good.

[heiner]

Hey, would be interested in the history of this change. I think we should probably merge the "rendering" modes here, but I would first like to understand why we added this mode here?