Map editor overhaul #33619

Qrox · 2019-08-28T15:08:16Z

Summary

SUMMARY: Interface "Map editor overhaul"

Purpose of change

The map editor had some very ancient code and some of its sub-menus were very hard to use -- For example, the terrain and furniture editing menu lacked filtering, so if one wanted to find a specific terrain/furniture they had to scroll through the whole list.

Describe the solution

Refactored the code for terrain, furniture, and trap editing. Their structures are surprisingly similar in term of map editing, so I made a function template unifying these three map features into a single map editing function, with minimal specialization for each type. uilist is used in this function template, and I made it support menu item description without automatic width/height so the menu can be positioned in their previous location.

The help text of the map editing functions are now composed using input context, and properly folded and printed in the info window. To make it more pretty when folded I added non-breaking spaces between the key names and action names. They are working correctly in-game, and Transifex also marks them with a non-empty symbol, so they can be properly translated.

To display info window title properly I made center_print support color-tagged text. To make center_print work properly with the title text containing literal angle brackets, I also made utf8_width not ignore non-tag angle brackets (< and >). It is only a temporary workaround, and these text related functions probably should ultimately support some escape sequences such as <lt> and <gt> to represent literal angle brackets.

To make the code easier to understand, the uilist callbacks are removed and action ids returned from uilist are used instead. This is accomplished by adding allow_additional to uilist to support returning from query() once a registered additional action is received.

Furthremore, fixed some issues with graphics refreshing. Now all help text are properly updating when entering a different menu. "HELP_KEYBINDINGS" is now sent to the callback and returned as an additional action in uilist::query(), so the map editing functions can properly refresh the display after exiting from the keybindings menu in several places.

Finally I also removed a bunch of unused variables and functions.

Additional context

Tested in wincurses and tiles.

esotericist

While I wholeheartedly endorse trying to make the debug map editor functional (I consider it essentially unusable at the moment), I'm a little unsure about some of the changes you've made in support of it.

Unfortunately, I'm not in a position to properly examine your changes to editmap (which in turn means I don't have a context for evaluating your uilist changes), so I am currently limiting my response to what I'm able to follow at this moment.

esotericist · 2019-08-28T16:11:23Z

src/catacharset.cpp

@@ -150,9 +150,13 @@ int utf8_width( const char *s, const bool ignore_tags )
            continue;
        }
        if( ignore_tags ) {
-            if( ch == '<' ) {
+            const char *const tag1 = "color_";


Cataclysm-DDA/src/catacharset.cpp

Lines 153 to 159 in 50fa905

const char *const tag1 = "color_";

const char *const tag2 = "/color>";

if( !inside_tag && ch == '<' &&

( strncmp( ptr, tag1, strlen( tag1 ) ) == 0 || strncmp( ptr, tag2, strlen( tag2 ) ) == 0 ) ) {

inside_tag = true;

} else if( inside_tag && ch == '>' ) {

Can you clarify the purpose of this logic? I understand the addition of inside_tag to the bottom conditional, but I'm unclear on the intent of the string comparisons.

It might be less performant, but it could make more sense to remove all the tags from the string.
Isn't remove_color_tags doing this actually?

the intent of the string comparisons.

For example, if we consider a string "< <color_cyan>Map editor</color> >", and set inside_tag to true on every occurrence of < (the previous logic), the characters that will be counted for text width will only be "Map editor >", without the starting bracket and space. If we check if < is followed by "color_" or "/color>", then it will properly count the starting characters. As I said in the OP this is only a temporary solution, but considering the state of how tags are handled currently in the many output functions, a proper fix would be complicated and should probably go in another PR.

I also think remove_color_tags would more clear here.

It would, but utf8_width needs fixing regardless since it's also called in other functions such as right_print. Maybe change utf8_width to also use remove_color_tags if ignore_tags is true?

esotericist · 2019-08-28T16:16:15Z

src/input.cpp

        //~ keybinding description for anykey
-        return string_format( pgettext( "keybinding", "[any] %s" ), text );
+        return string_format( pgettext( "keybinding", "[any]\u00A0%s" ), text );


While I'm not an expert on how we handle translation, I don't think this is a good idea.

You don't currently have a translation comment to indicate to translators there is a non-breaking space here, but even if you did add that, translators might not be prepared to preserve it in their versions of these strings.

Further, I'm not clear on the value of having it in the first place.

translators might not be prepared to preserve it in their versions of these strings.

Transifex will show the non-breaking space using a special symbol. In the worst case when the translator is not paying attention, it will be translated into a normal space, which is just what it was before this change.

Further, I'm not clear on the value of having it in the first place.

[a] press a key, [b] | <- window boundary press another key |

compared with

[a] press a key, | [b] press another key|

I believe the second one is more aesthetically pleasing.

esotericist · 2019-08-28T16:16:28Z

src/input.cpp

@@ -795,14 +796,20 @@ std::string input_context::get_desc( const std::string &action_descriptor,

    if( na ) {
        //~ keybinding description for unbound or disabled keys
-        return string_format( pgettext( "keybinding", "[n/a] %s" ), text );
+        return string_format( pgettext( "keybinding", "[n/a]\u00A0%s" ), text );


esotericist · 2019-08-28T16:16:35Z

src/input.cpp

    } else {
        //~ keybinding description for bound keys
-        return string_format( pgettext( "keybinding", "[%s] %s" ),
+        return string_format( pgettext( "keybinding", "[%s]\u00A0%s" ),


BevapDin · 2019-08-28T16:44:45Z

src/editmap.cpp

+    int veh_in = -1;
+    const optional_vpart_position vp = g->m.veh_at( target );
+    if( vp ) {
+        veh_in = vp->is_inside();


is_inside returns a bool, so veh_in should probably be a bool as well.

BevapDin · 2019-08-28T16:46:57Z

src/editmap.cpp

+    const auto &map_cache = g->m.get_cache( target.z );
+
+    mvwprintw( w_info, point( 1, off++ ), _( "dist: %d u_see: %d v_in: %d scent: %d" ),
+               rl_dist( g->u.pos(), target ), static_cast<int>( g->u.sees( target ) ),


Suggested change

rl_dist( g->u.pos(), target ), static_cast<int>( g->u.sees( target ) ),

rl_dist( g->u.pos(), target ), g->u.sees( target ) ? _("visible"): "_("invisible"),

Most people without a programming background won't get what the 1 or 0 there means.

BevapDin · 2019-08-28T16:47:58Z

src/editmap.cpp

-                     );
-            off++; // 10ish
-        }
+    mvwprintw( w_info, point( 1, off ), "%s %s", g->m.features( target ).c_str(), extras );


Suggested change

mvwprintw( w_info, point( 1, off ), "%s %s", g->m.features( target ).c_str(), extras );

mvwprintw( w_info, point( 1, off ), "%s %s", g->m.features( target ), extras );

These code were working before so I didn't check them, only changed their indentation because I removed an if clause... Your suggestions are viable though, so I'll apply them.

vbruhn · 2019-09-10T11:47:36Z

There seems to be a bug with the terrain editor changing an area of tiles, when just going with 'TAB' and 'Enter' through construction.
I am on Build 9601, and wanted to delete a few nasty walls from a farm-house. The first couple of tiles worked fine, after a couple of changes it changed at least 4 tiles in a row (other directions were already floor tile).
Reset of the world, made a backup of a similar Farm-Building, watched my steps, not accidentily hit 'm' ... and it happened again.

Regards, Volker

Qrox · 2019-09-10T12:19:48Z

Could not reproduce what you described. TAB switches the mode to shape editing on the first hit and shape moving on the second, perhaps you accidentally hit a directional key in the shape editing mode? By the way, there's already another PR by me that will add preview to the debug map editor which should make it less likely to unknowingly change the shape, but it's not merged yet.

vbruhn · 2019-09-10T13:12:44Z

Thanks for the quick response.
To make my (!) issue more clearly, I attach a screenshot.
Maybe it is on purpose, that the map editor creates patterns now. But this is not obvious for me.

My steps:
Debug-Menu: m - M ... (move cursor) - g - (select floor tile) - Enter - Tab - (move cursor) - Enter - Tab - (move cursor) ...
In the previous version, this changed only the selected tile (though the key to change cursor position was a different one?)
The shown changes in the image were only a few clicks ... 2 on the right, 4 for the middle, 2 for the left.
Then I wanted to change only the top right tile to be an empty door frame ... and it changed the whole pattern from floor to door frame.

I am sorry, but I do not understand, how I could possibly change a single tile at the moment :)
Regards, Volker

P.S.: Feature Request: ;-) It would be nice to search for the terrain-IDs ... I know 156 for floor, but searching for floor has too many options left. Thanks :)

Edit ... deleted a msileading bit of text

vbruhn · 2019-09-10T13:33:17Z

I went into an old version to understand my misunderstanding. Sorry for bothering you with a non-existing bug.

Edit:
For dumbheads like me, it would be valuable to have the (m) option to be renamed to (m)ove cursor, for example.
The (Tab)-menu could be renamed to define shape, or the like.
Inside the (Tab)-menu it would make more sense to me to rename the Re(s)ize menu to Re(s)hape

Thanks :) for your attention
Volker

Qrox · 2019-09-10T13:52:33Z

You can search for the string id, they are already listed along with the terrain name (t_floor for floor, for example). I'm not sure if searching for the integer id is a good idea, because if my understanding about integer id is correct they might change across different game version or even saves.

vbruhn · 2019-09-10T14:16:34Z

You can search for the string id, they are already listed along with the terrain name (t_floor for floor, for example). I'm not sure if searching for the integer id is a good idea, because if my understanding about integer id is correct they might change across different game version or even saves.

Good point ... it would be nice then, when hovering over a tile, that the information window would give that addtional information, right now it shows the (current) ID and the name, not the terrain name.

Qrox · 2019-09-10T15:23:06Z

Yeah, it would be better if it shows the string id of the terrain in the info window. I'll look into it. For now you can move the cursor to a tile and then enter the terrain editing menu. It will place the menu cursor to the current terrain of that tile (same for furnitures and traps).

Refactor map editor code

50fa905

alkemann added Map / Mapgen Overmap, Mapgen, Map extras, Map display Info / User Interface Game - player communication, menus, etc. labels Aug 28, 2019

ghost requested review from esotericist and removed request for esotericist August 28, 2019 15:56

esotericist reviewed Aug 28, 2019

View reviewed changes

BevapDin reviewed Aug 28, 2019

View reviewed changes

Qrox added 2 commits August 29, 2019 21:20

Apply suggestions from reviews

8e5a722

Use remove_color_tags in utf8_width to remove color tags

a479cde

ZhilkinSerg self-assigned this Sep 1, 2019

Qrox force-pushed the debug-edit-overhaul branch from df3d3b1 to a479cde Compare September 1, 2019 15:40

Qrox mentioned this pull request Sep 2, 2019

Implement debug map editor preview for Tiles #33760

Merged

ZhilkinSerg merged commit 094ef8e into CleverRaven:master Sep 5, 2019

ZhilkinSerg removed their assignment Sep 5, 2019

Qrox deleted the debug-edit-overhaul branch September 5, 2019 10:00

kevingranade mentioned this pull request Sep 6, 2019

Fix main menu center spacing #33835

Merged

Aloxaf mentioned this pull request Sep 13, 2019

Hotkey conflict with "edit furniture" and "pan up-right" #34003

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map editor overhaul #33619

Map editor overhaul #33619

Qrox commented Aug 28, 2019

esotericist left a comment

esotericist Aug 28, 2019

AMurkin Aug 28, 2019

Qrox Aug 29, 2019

ZhilkinSerg Aug 30, 2019

Qrox Aug 30, 2019

esotericist Aug 28, 2019

Qrox Aug 29, 2019 •

edited

Loading

esotericist Aug 28, 2019

esotericist Aug 28, 2019

BevapDin Aug 28, 2019

BevapDin Aug 28, 2019

BevapDin Aug 28, 2019

Qrox Aug 29, 2019

vbruhn commented Sep 10, 2019

Qrox commented Sep 10, 2019 •

edited

Loading

vbruhn commented Sep 10, 2019 •

edited

Loading

vbruhn commented Sep 10, 2019 •

edited

Loading

Qrox commented Sep 10, 2019

vbruhn commented Sep 10, 2019 •

edited

Loading

Qrox commented Sep 10, 2019

	const char *const tag1 = "color_";
	const char *const tag2 = "/color>";
	if( !inside_tag && ch == '<' &&
	( strncmp( ptr, tag1, strlen( tag1 ) ) == 0 \|\| strncmp( ptr, tag2, strlen( tag2 ) ) == 0 ) ) {

	inside_tag = true;
	} else if( inside_tag && ch == '>' ) {

	rl_dist( g->u.pos(), target ), static_cast<int>( g->u.sees( target ) ),
	rl_dist( g->u.pos(), target ), g->u.sees( target ) ? _("visible"): "_("invisible"),

	mvwprintw( w_info, point( 1, off ), "%s %s", g->m.features( target ).c_str(), extras );
	mvwprintw( w_info, point( 1, off ), "%s %s", g->m.features( target ), extras );

Map editor overhaul #33619

Map editor overhaul #33619

Conversation

Qrox commented Aug 28, 2019

Summary

Purpose of change

Describe the solution

Additional context

esotericist 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

Qrox Aug 29, 2019 • edited Loading

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

vbruhn commented Sep 10, 2019

Qrox commented Sep 10, 2019 • edited Loading

vbruhn commented Sep 10, 2019 • edited Loading

vbruhn commented Sep 10, 2019 • edited Loading

Qrox commented Sep 10, 2019

vbruhn commented Sep 10, 2019 • edited Loading

Qrox commented Sep 10, 2019

Qrox Aug 29, 2019 •

edited

Loading

Qrox commented Sep 10, 2019 •

edited

Loading

vbruhn commented Sep 10, 2019 •

edited

Loading

vbruhn commented Sep 10, 2019 •

edited

Loading

vbruhn commented Sep 10, 2019 •

edited

Loading