2019-12-09 05:22 EST


View Issue Details Jump to Notes ] Related Changesets ]
IDProjectCategoryView StatusLast Update
0003071FSSCPuser interfacepublic2014-07-24 23:42
Reporterngld 
Assigned To 
PrioritynormalSeverityfeatureReproducibilityN/A
StatusresolvedResolutionfixed 
Product Version 
Target Version3.7.2Fixed in Version3.7.2 
Summary0003071: New Mainhall capabilities
DescriptionSee this thread for details: http://www.hard-light.net/forums/index.php?topic=86888.0

The patch was aleady reviewed by AdmiralRalwood / MageKing17.

The patch is here: http://pastebin.com/XugqKG52
An example mod is here: http://dev.tproxy.de/fs2/test-mainhall.zip
TagsNo tags attached.
Attached Files
  • patch file icon mainhall_caps.patch (18,835 bytes) 2014-07-07 09:36 -
    Index: code/menuui/mainhallmenu.cpp
    ===================================================================
    --- code/menuui/mainhallmenu.cpp	(revision 10896)
    +++ code/menuui/mainhallmenu.cpp	(working copy)
    @@ -33,6 +33,7 @@
     #include "menuui/fishtank.h"
     #include "mission/missioncampaign.h"
     #include "parse/parselo.h"
    +#include "parse/scripting.h"
     #include "network/multiui.h"
     #include "network/multiutil.h"
     #include "network/multi_voice.h"
    @@ -151,8 +152,7 @@
     // ----------------------------------------------------------------------------
     // SNAZZY MENU stuff
     //
    -#define NUM_MAIN_HALL_REGIONS 10
    -#define NUM_MAIN_HALL_MOUSE_REGIONS 6
    +#define NUM_MAIN_HALL_MAX_REGIONS 20
     
     // region mask #'s (identifiers)
     #define EXIT_REGION				0
    @@ -165,9 +165,32 @@
     #define LOAD_MISSION_REGION		11
     #define QUICK_START_REGION		12
     #define SKILL_LEVEL_REGION		13
    +#define SCRIPT_REGION			14
    +#define START_REGION			15
     
    +struct main_hall_region_info {
    +	int mask;
    +	char *name;
    +};
    +
    +main_hall_region_info Main_hall_region_map[] = {
    +	{ EXIT_REGION, "Exit" },
    +	{ BARRACKS_REGION, "Barracks" },
    +	{ READY_ROOM_REGION, "Readyroom" },
    +	{ TECH_ROOM_REGION, "Techroom" },
    +	{ OPTIONS_REGION, "Options" },
    +	{ CAMPAIGN_ROOM_REGION, "Campaigns" },
    +	{ MULTIPLAYER_REGION, "Multiplayer" },
    +	{ LOAD_MISSION_REGION, "Load Mission" },
    +	{ QUICK_START_REGION, "Quickstart" },
    +	{ SKILL_LEVEL_REGION, "Skilllevel" },
    +	{ SCRIPT_REGION, "Script" },
    +	{ START_REGION, "Start" },
    +	{ -1, NULL }
    +};
    +
     // all the menu regions in the main hall
    -MENU_REGION Main_hall_region[NUM_MAIN_HALL_REGIONS];
    +MENU_REGION Main_hall_region[NUM_MAIN_HALL_MAX_REGIONS];
     
     // # of regions (options) on this screen. parsed from a table
     int Main_hall_num_options;
    @@ -395,7 +418,6 @@
     	}
     
     	int idx;
    -	char temp[100], whee[100];
     	SCP_string main_hall_to_load;
     
     	// reparse the table here if the relevant cmdline flag is set
    @@ -423,20 +445,24 @@
     
     	// create the snazzy interface and load up the info from the table
     	snazzy_menu_init();
    -	read_menu_tbl(NOX("MAIN HALL"), temp, whee, Main_hall_region, &Main_hall_num_options, 0);
    -
    +	
     	// assign the proper main hall data
     	Assert(main_hall_get_pointer(main_hall_to_load) != NULL);
     	Main_hall = main_hall_get_pointer(main_hall_to_load);
     
    -	// tooltip strings
    -	Main_hall->region_descript.at(0) = XSTR( "Exit FreeSpace 2", 353);
    -	Main_hall->region_descript.at(1) = XSTR( "Barracks - Manage your FreeSpace 2 pilots", 354);
    -	Main_hall->region_descript.at(2) = XSTR( "Ready room - Start or continue a campaign", 355);
    -	Main_hall->region_descript.at(3) = XSTR( "Tech room - View specifications of FreeSpace 2 ships and weaponry", 356);
    -	Main_hall->region_descript.at(4) = XSTR( "Options - Change your FreeSpace 2 options", 357);
    -	Main_hall->region_descript.at(5) = XSTR( "Campaign Room - View all available campaigns", 358);
    -	Main_hall->region_descript.at(6) = XSTR( "Multiplayer - Start or join a multiplayer game", 359);
    +	// check if we have to change the ready room's description
    +	if (Player->flags & PLAYER_FLAGS_IS_MULTI) {
    +		if(Main_hall->default_readyroom) {
    +			Main_hall->regions[2].description = XSTR( "Multiplayer - Start or join a multiplayer game", 359);
    +		}
    +	}
    +	
    +	// Read the menu regions from mainhall.tbl
    +	SCP_vector<main_hall_region>::iterator it;
    +	for (it = Main_hall->regions.begin(); Main_hall->regions.end() != it; ++it) {
    +		snazzy_menu_add_region(Main_hall_region + (it - Main_hall->regions.begin()), it->description.c_str(), it->mask, 0, -1);
    +	}
    +	Main_hall_num_options = Main_hall->regions.size();
     
     	// init tooltip shader						// nearly black
     	gr_create_shader(&Main_hall_tooltip_shader, 5, 5, 5, 168);
    @@ -637,7 +663,8 @@
      */
     void main_hall_do(float frametime)
     {
    -	int code, key, snazzy_action;
    +	int code, key, snazzy_action, region_action = -1;
    +	SCP_vector<main_hall_region>::iterator it;
     
     	// set the screen scale to the main hall's dimensions
     	gr_set_screen_scale(Main_hall_bitmap_w, Main_hall_bitmap_h, Main_hall->zoom_area_width, Main_hall->zoom_area_height);
    @@ -692,11 +719,38 @@
     	// do any processing based upon what happened to the snazzy menu
     	switch (snazzy_action) {
     		case SNAZZY_OVER:
    -			main_hall_handle_mouse_location(code);
    +			for (it = Main_hall->regions.begin(); Main_hall->regions.end() != it; ++it) {
    +				if (it->mask == code) {
    +					main_hall_handle_mouse_location(it - Main_hall->regions.begin());
    +					break;
    +				}
    +			}
    +			
     			break;
     
     		case SNAZZY_CLICKED:
    -			switch (code) {
    +			if (code == ESC_PRESSED) {
    +				region_action = ESC_PRESSED;
    +			} else {
    +				for (it = Main_hall->regions.begin(); Main_hall->regions.end() != it; ++it) {
    +					if (it->mask == code) {
    +						region_action = it->action;
    +						break;
    +					}
    +				}
    +				
    +				if (region_action == -1) {
    +					Error(LOCATION, "Region %d doesn't have an action!", code);
    +				} else if (region_action == START_REGION) {
    +					if (Player->flags & PLAYER_FLAGS_IS_MULTI) {
    +						region_action = MULTIPLAYER_REGION;
    +					} else {
    +						region_action = READY_ROOM_REGION;
    +					}
    +				}
    +			}
    +			
    +			switch (region_action) {
     				// clicked on the exit region
     				case EXIT_REGION:
     					gamesnd_play_iface(SND_IFACE_MOUSE_CLICK);
    @@ -705,17 +759,16 @@
     
     				// clicked on the readyroom region
     				case READY_ROOM_REGION:
    -					if (Player->flags & PLAYER_FLAGS_IS_MULTI) {
    -						gamesnd_play_iface(SND_IFACE_MOUSE_CLICK);
    -						main_hall_do_multi_ready();
    +					// Make sure we aren't in multi mode.
    +					Player->flags = Player->flags ^ PLAYER_FLAGS_IS_MULTI;
    +					Game_mode = GM_NORMAL;
    +					
    +					if (strlen(Main_hall_campaign_cheat)) {
    +						gameseq_post_event(GS_EVENT_CAMPAIGN_CHEAT);
     					} else {
    -						if (strlen(Main_hall_campaign_cheat)) {
    -							gameseq_post_event(GS_EVENT_CAMPAIGN_CHEAT);
    -						} else {
    -							gameseq_post_event(GS_EVENT_NEW_CAMPAIGN);
    -						}
    -						gamesnd_play_iface(SND_IFACE_MOUSE_CLICK);
    +						gameseq_post_event(GS_EVENT_NEW_CAMPAIGN);
     					}
    +					gamesnd_play_iface(SND_IFACE_MOUSE_CLICK);
     					break;
     
     				// clicked on the tech room region
    @@ -732,23 +785,20 @@
     
     				// clicked on the campaign toom region
     				case CAMPAIGN_ROOM_REGION:
    -					if (Player->flags & PLAYER_FLAGS_IS_MULTI) {
    -						gamesnd_play_iface(SND_IFACE_MOUSE_CLICK);
    -						main_hall_set_notify_string(XSTR( "Campaign Room not valid for multiplayer pilots", 366));
    -					} else {
    -						gamesnd_play_iface(SND_IFACE_MOUSE_CLICK);
    -						gameseq_post_event(GS_EVENT_CAMPAIGN_ROOM);
    -					}
    +					gamesnd_play_iface(SND_IFACE_MOUSE_CLICK);
    +					gameseq_post_event(GS_EVENT_CAMPAIGN_ROOM);
     					break;
     
     				// clicked on the multiplayer region
     				case MULTIPLAYER_REGION:
    -					if (Player->flags & PLAYER_FLAGS_IS_MULTI) {
    -						// NOTE : this isn't a great thing to be calling this anymore. But we'll leave it for now
    -						gameseq_post_event(GS_EVENT_MULTI_JOIN_GAME);
    -					} else {
    -						main_hall_set_notify_string(XSTR( "Not a valid multiplayer pilot!!", 367));
    -					}
    +					// Make sure we are in multi mode.
    +					Player->flags |= PLAYER_FLAGS_IS_MULTI;
    +					Game_mode = GM_MULTIPLAYER;
    +					
    +					main_hall_do_multi_ready();
    +					
    +					// NOTE : this isn't a great thing to be calling this anymore. But we'll leave it for now
    +					gameseq_post_event(GS_EVENT_MULTI_JOIN_GAME);
     					break;
     
     				// load mission key was pressed
    @@ -758,20 +808,16 @@
     				// quick start a game region
     				case QUICK_START_REGION:
     			#if !defined(NDEBUG)
    -					if (Player->flags & PLAYER_FLAGS_IS_MULTI) {
    -						main_hall_set_notify_string(XSTR( "Quick Start not valid for multiplayer pilots", 369));
    +					if (Num_recent_missions > 0) {
    +						strcpy_s(Game_current_mission_filename, Recent_missions[0]);
     					} else {
    -						if (Num_recent_missions > 0) {
    -							strcpy_s(Game_current_mission_filename, Recent_missions[0]);
    -						} else {
    -							if (mission_load_up_campaign()) {
    -								main_hall_set_notify_string(XSTR( "Campaign file is currently unavailable", 1606));
    -							}
    -							strcpy_s(Game_current_mission_filename, Campaign.missions[0].name);
    +						if (mission_load_up_campaign()) {
    +							main_hall_set_notify_string(XSTR( "Campaign file is currently unavailable", 1606));
     						}
    -						Campaign.current_mission = -1;
    -						gameseq_post_event(GS_EVENT_START_GAME_QUICK);
    +						strcpy_s(Game_current_mission_filename, Campaign.missions[0].name);
     					}
    +					Campaign.current_mission = -1;
    +					gameseq_post_event(GS_EVENT_START_GAME_QUICK);
     			#endif
     					break;
     
    @@ -799,11 +845,17 @@
     						help_overlay_set_state(Main_hall_overlay_id,gr_screen.res,0);
     					}
     					break;
    +				
    +				// custom action
    +				case SCRIPT_REGION:
    +					const char *lua = it->lua_action.c_str();
    +					Script_system.EvalString(lua, NULL, NULL, lua);
    +					break;
     			} // END switch (code)
     
     			// if the escape key wasn't pressed handle any mouse position related events
     			if (code != ESC_PRESSED) {
    -				main_hall_handle_mouse_location(code);
    +				main_hall_handle_mouse_location((region_action == -1 ? -1 : it - Main_hall->regions.begin()));
     			}
     			break;
     
    @@ -1211,8 +1263,8 @@
     	if (Main_hall_frame_skip) {
     		return;
     	}
    -
    -	if (cur_region > NUM_MAIN_HALL_MOUSE_REGIONS) {
    +	
    +	if (cur_region > (int)Main_hall->regions.size() - 1) {
     		// MWA -- inserted return since Int3() was tripped when hitting L from main menu.
     		return;
     	}
    @@ -1262,7 +1314,7 @@
     	}
     
     	// don't do anything if there are no animations to play
    -	else if(Main_hall_door_anim.size() == 0)
    +	else if(region > (int) Main_hall_door_anim.size() - 1)
     	{
     		return;
     	}
    @@ -1303,7 +1355,7 @@
     	}
     
     	// don't do anything if there are no animations to play
    -	else if(Main_hall_door_anim.size() == 0)
    +	else if(region > (int) Main_hall_door_anim.size() - 1)
     	{
     		return;
     	}
    @@ -1351,7 +1403,7 @@
     	if (!Main_hall_right_click) {
     		if (mouse_down(MOUSE_RIGHT_BUTTON)) {
     			// cycle through the available regions
    -			if (Main_hall_last_clicked_region == NUM_MAIN_HALL_MOUSE_REGIONS - 1) {
    +			if (Main_hall_last_clicked_region == (int) Main_hall_door_anim.size() - 1) {
     				new_region = 0;
     			} else {
     				new_region = Main_hall_last_clicked_region + 1;
    @@ -1579,7 +1631,7 @@
      */
     void main_hall_maybe_blit_tooltips()
     {
    -	int w, h, text_index;
    +	int w, h;
     
     	// if we're over no region - don't blit anything
     	if (Main_hall_mouse_region < 0) {
    @@ -1586,24 +1638,18 @@
     		return;
     	}
     
    -	// get the index of the proper text to be using
    -	if (Main_hall_mouse_region == READY_ROOM_REGION) {
    -		// if this is a multiplayer pilot, the ready room region becomes the multiplayer region
    -		if (Player->flags & PLAYER_FLAGS_IS_MULTI){
    -			text_index = NUM_REGIONS - 1;
    -		} else {
    -			text_index = READY_ROOM_REGION;
    -		}
    -	} else {
    -		text_index = Main_hall_mouse_region;
    +	if (Main_hall_mouse_region >= (int) Main_hall->regions.size()) {
    +		Error(LOCATION, "Missing region description for index %d!\n", Main_hall_mouse_region);
     	}
     
     	// set the color and blit the string
     	if (!help_overlay_active(Main_hall_overlay_id)) {
    +		const char* desc = Main_hall->regions[Main_hall_mouse_region].description.c_str();
    +		
     		int old_font = gr_get_current_fontnum();
     		gr_set_font(Main_hall->font);
     		// get the width of the string
    -		gr_get_string_size(&w, &h, Main_hall->region_descript.at(text_index));
    +		gr_get_string_size(&w, &h, desc);
     		int text_y;
     		if (Main_hall->region_yval == -1) {
     			text_y = gr_screen.max_h_unscaled - ((gr_screen.max_h_unscaled - gr_screen.max_h_unscaled_zoomed) / 2) - Main_hall->tooltip_padding - h;
    @@ -1611,12 +1657,12 @@
     			text_y = Main_hall->region_yval;
     		}
     		int shader_y = text_y - (Main_hall->tooltip_padding);	// subtract more to pull higher
    -
    +		
     		gr_set_shader(&Main_hall_tooltip_shader);
     		gr_shade(0, shader_y, gr_screen.max_w_unscaled, (gr_screen.max_h_unscaled - shader_y), GR_RESIZE_MENU);
     
     		gr_set_color_fast(&Color_bright_white);
    -		gr_string((gr_screen.max_w_unscaled - w)/2, text_y, Main_hall->region_descript.at(text_index), GR_RESIZE_MENU);
    +		gr_string((gr_screen.max_w_unscaled - w)/2, text_y, desc, GR_RESIZE_MENU);
     
     		gr_set_font(old_font);
     	}
    @@ -1876,7 +1922,6 @@
     		m.door_anim_coords.clear();
     		m.door_sounds.clear();
     		m.door_sound_pan.clear();
    -		m.region_descript.clear();
     	}
     
     	SCP_vector<int> temp;
    @@ -1901,12 +1946,29 @@
     		// door_sound_pan
     		m.door_sound_pan.push_back(0.0f);
     	}
    +}
     
    -	// region_descript
    -	for (idx = 0; idx < NUM_REGIONS; idx++) {
    -		m.region_descript.push_back(NULL);
    +void region_info_init(main_hall_defines &m)
    +{
    +	if (Cmdline_reparse_mainhall) {
    +		m.regions.clear();
     	}
    -
    +	
    +	main_hall_region defaults[] = {
    +		{0, XSTR( "Exit FreeSpace 2", 353), EXIT_REGION, ""},
    +		{1, XSTR( "Barracks - Manage your FreeSpace 2 pilots", 354), BARRACKS_REGION, ""},
    +		{2, XSTR( "Ready room - Start or continue a campaign", 355), START_REGION, ""},
    +		{3, XSTR( "Tech room - View specifications of FreeSpace 2 ships and weaponry", 356), TECH_ROOM_REGION, ""},
    +		{4, XSTR( "Options - Change your FreeSpace 2 options", 357), OPTIONS_REGION, ""},
    +		{5, XSTR( "Campaign Room - View all available campaigns", 358), CAMPAIGN_ROOM_REGION, ""}
    +	};
    +	
    +	for (int idx = 0; idx < 6; idx++) {
    +		m.regions.push_back(defaults[idx]);
    +	}
    +	
    +	// XSTR( "Multiplayer - Start or join a multiplayer game", 359)
    +	m.default_readyroom = true;
     }
     
     /**
    @@ -1935,6 +1997,7 @@
     	int num_resolutions = 2;
     	unsigned int count;
     	char temp_string[MAX_FILENAME_LEN];
    +	SCP_string temp_scp_string;
     
     	if ((rval = setjmp(parse_abort)) != 0) {
     		mprintf(("TABLES: Unable to parse '%s'!  Error code = %i.\n", filename, rval));
    @@ -2165,7 +2228,75 @@
     					m->misc_anim_sound_flag.at(idx).push_back(0);
     				}
     			}
    +			
    +			region_info_init(*m);
    +			
    +			int mask;
    +			for (idx = 0; optional_string("+Door mask value:"); idx++) {
    +				// door mask
    +				stuff_string(temp_string, F_RAW, MAX_FILENAME_LEN);
    +				
    +				mask = (int) strtol(temp_string, NULL, 0);
    +				mask = 255 - mask;
    +				
    +				if(idx >= (int) m->regions.size()) {
    +					m->regions.resize(idx + 1);
    +				}
    +				m->regions[idx].mask = mask;
    +			}
    +			
    +			for (idx = 0; optional_string("+Door action:"); idx++) {
    +				// door action
    +				
    +				if(idx >= (int) m->regions.size()) {
    +					m->regions.resize(idx + 1);
    +				}
    +				
    +				if (optional_string("Script")) {
    +					m->regions[idx].action = SCRIPT_REGION;
    +					stuff_string(m->regions[idx].lua_action, F_RAW);
    +				} else {
    +					stuff_string(temp_scp_string, F_RAW);
    +					
    +					int action = -1;
    +					for (int i = 0; Main_hall_region_map[i].name != NULL; i++) {
    +						if (temp_scp_string == Main_hall_region_map[i].name) {
    +							action = Main_hall_region_map[i].mask;
    +							break;
    +						}
    +					}
    +					
    +					if (action == -1) {
    +						SCP_string err_msg = "";
    +						for (int i = 0; Main_hall_region_map[i].name != NULL; i++) {
    +							err_msg += ", ";
    +							err_msg += Main_hall_region_map[i].name;
    +						}
    +						
    +						Error(LOCATION, "Unkown Door Region '%s'! Expected one of: %s", temp_scp_string.c_str(), err_msg.substr(2).c_str());
    +					}
    +					
    +					m->regions[idx].action = action;
    +				}
    +			}
     
    +			for (idx = 0; optional_string("+Door description:"); idx++) {
    +				// region description (tooltip)
    +				stuff_string(temp_scp_string, F_MESSAGE);
    +
    +				if (temp_scp_string != "default") {
    +					if(idx >= (int) m->regions.size()) {
    +						m->regions.resize(idx + 1);
    +					}
    +					
    +					m->regions[idx].description = temp_scp_string;
    +					
    +					if (idx == 2) {
    +						m->default_readyroom = false;
    +					}
    +				}
    +			}
    +
     			// door animations
     			required_string("+Num Door Animations:");
     			stuff_int(&m->num_door_animations);
    Index: code/menuui/mainhallmenu.h
    ===================================================================
    --- code/menuui/mainhallmenu.h	(revision 10896)
    +++ code/menuui/mainhallmenu.h	(working copy)
    @@ -16,6 +16,13 @@
     // CommanderDJ - this is now dynamic
     // #define MAIN_HALLS_MAX			10
     
    +typedef struct main_hall_region {
    +	int mask;
    +	SCP_string description;
    +	int action;
    +	SCP_string lua_action;
    +} main_hall_region;
    +
     typedef struct main_hall_defines {
     	// mainhall name identifier
     	SCP_string name;
    @@ -115,8 +122,10 @@
     	// font used for the tooltips, version number, etc.
     	int font;
     
    -	// text (tooltip) description
    -	SCP_vector<const char*> region_descript;
    +	// action
    +	SCP_vector<main_hall_region> regions;
    +	
    +	bool default_readyroom;
     
     	// num pixels shader is above/below tooltip text
     	int tooltip_padding;
    Index: code/menuui/snazzyui.cpp
    ===================================================================
    --- code/menuui/snazzyui.cpp	(revision 10896)
    +++ code/menuui/snazzyui.cpp	(working copy)
    @@ -160,7 +160,7 @@
     //
     //
     
    -void snazzy_menu_add_region(MENU_REGION* region, char* text, int mask, int key, int click_sound)
    +void snazzy_menu_add_region(MENU_REGION* region, const char* text, int mask, int key, int click_sound)
     {
     	region->mask = mask;
     	region->key = key;
    Index: code/menuui/snazzyui.h
    ===================================================================
    --- code/menuui/snazzyui.h	(revision 10896)
    +++ code/menuui/snazzyui.h	(working copy)
    @@ -30,7 +30,7 @@
     
     int snazzy_menu_do(ubyte *data, int mask_w, int mask_h, int num_regions, MENU_REGION *regions, int *action, int poll_key = 1, int *key = NULL);
     void read_menu_tbl(char *menu_name, char *bkg_filename, char *mask_filename, MENU_REGION *regions, int* num_regions, int play_sound=1);
    -void snazzy_menu_add_region(MENU_REGION *region, char* text, int mask, int key, int click_sound = -1);
    +void snazzy_menu_add_region(MENU_REGION *region, const char* text, int mask, int key, int click_sound = -1);
     
     void snazzy_menu_init();		// Call the first time a snazzy menu is inited
     void snazzy_menu_close();
    Index: code/parse/scripting.cpp
    ===================================================================
    --- code/parse/scripting.cpp	(revision 10896)
    +++ code/parse/scripting.cpp	(working copy)
    @@ -1069,7 +1069,7 @@
     	return 1;
     }
     
    -bool script_state::EvalString(char* string, char *format, void *rtn, char *debug_str)
    +bool script_state::EvalString(const char* string, char *format, void *rtn, const char *debug_str)
     {
     	char lastchar = string[strlen(string)-1];
     
    Index: code/parse/scripting.h
    ===================================================================
    --- code/parse/scripting.h	(revision 10896)
    +++ code/parse/scripting.h	(working copy)
    @@ -190,7 +190,7 @@
     	void RemHookVars(unsigned int num, ...);
     
     	//***Hook creation functions
    -	bool EvalString(char* string, char *format=NULL, void *rtn=NULL, char *debug_str=NULL);
    +	bool EvalString(const char* string, char *format=NULL, void *rtn=NULL, const char *debug_str=NULL);
     	void ParseChunk(script_hook *dest, char* debug_str=NULL);
     	bool ParseCondition(const char *filename="<Unknown>");
     
    
    patch file icon mainhall_caps.patch (18,835 bytes) 2014-07-07 09:36 +

-Relationships
+Relationships

-Notes

~0015970

MjnMixael (manager)

I have also tested the features successfully.

~0015990

Goober5000 (administrator)

Coded, reviewed, and tested... good enough for me. I'll give it a once-over and then commit it.

~0015993

Goober5000 (administrator)

It's not the most efficiently coded patch, but the mainhall code isn't performance-critical so that doesn't matter as much. (The resizing of the main hall vector is a bit kludgy but there's nothing actually wrong with it.)

Though before I commit the patch, I want to ask about the region_entry_init() method. It was added in this patch but isn't referenced anywhere in the codebase. Did you forget to add function calls somewhere?

~0015999

ngld (reporter)

You're right, you can remove that function. I used it in an earlier version of region_info_init() but it's not needed anymore.

Any hints on how to make the patch more efficient?

~0016033

Goober5000 (administrator)

Several things, nothing game-breaking, but taken together they could be refined:

1. Referencing SCP_vector elements is done more efficiently using the [] operator rather than the at() function; furthermore, at() should only be used when you have set up exception handling for range checking, which is not the case here.

2. Iterating through SCP_vectors is done more efficiently using vector iterators (search for "++ii" and you'll find many examples).

3. When bridging between APIs that use char* vs. const char*, use const_cast<>() rather than C-style casts. (Also, you might want to correct the const-ness of the function if appropriate, but this can easily blow up into a huge task, so it's not something crucial to worry about.)

4. Your region_entry_init function passed SCP_strings by copying them, whereas it should have used const SCP_string& to pass them by reference. (But if the function is to be removed then that no longer matters.)

5. Your technique of adding an element to a SCP_vector via resizing the vector by 1 and then assigning the element via index is very kludgy. Instead, just use push_back() to place the element at the end of the vector.

~0016036

ngld (reporter)

I've attached a new version of the patch.
I used the at() function since the existing code in mainhallmenu.cpp does it that way. The same is true for vector iterators.

I've resolved both (char*) casts by changing the related declarations.
The first case is snazzy_menu_add_region() and the second is script_state::EvalString(). Both functions don't change the string so it should be fine to change the parameter to const char*.

I haven't changed the resize() calls since the SCP_vector for the menu regions always contains the default 6 regions. The first 6 times a directive is read, it overwrites the default regions and only after that I have to resize the vector.

if(idx >= (int) m->regions.size()) {
    m->regions.resize(idx + 1);
}
m->regions[idx].description = temp_scp_string;

looks nicer to me than

if(idx >= (int) m->regions.size()) {
    main_hall_region r;
    r.description = temp_scp_string;
    m->regions.push_back(r);
} else {
    m->regions[idx].description = temp_scp_string;
}

~0016110

niffiwan (developer)

*pokes Goober*

Do you have any further comments re: the updated patch? :)

From a quick look I've only got one comment; this could probably use a validity check that the input evaluates to >= 0 && <= 255.

for (idx = 0; optional_string("+Door mask value:"); idx++) {

~0016113

Goober5000 (administrator)

I haven't forgotten, don't worry. :)

Will try to give this a look ASAP.

~0016121

Goober5000 (administrator)

Ok, a few comments:

1) There's no need for this:
Main_hall_region + (it - Main_hall->regions.begin())
To access the object which is being iterated over, use the asterisk: *it. Or, use &(*it) if you need the address of the object.

If all you need is the index of the iterator, then this...
it - Main_hall->regions.begin()
...is acceptable.

2) For clarity, this...
if (region > size - 1)
...is equivalent to this...
if (region >= size)

3) When building err_msg at around line 2270, instead of printing the string at substr(2), just add a check to skip adding the comma if i == 0.

4) Main_hall_num_options doesn't appear to be used for anything, it's just assigned and passed around. It should be removed, since .size() is used elsewhere.

In other respects, ngld, your patch is good, and your explanation about resizing +1 is fine.

One caveat: I haven't actually checked the *functionality* of the patch, just the code smell. So someone should load up this version of the patch and see if it works (presumably ngld, and presumably he already has).

~0016122

MageKing17 (developer)

Currently attached patch contains "Player->flags = Player->flags ^ PLAYER_FLAGS_IS_MULTI;" when the pastebin link uses "&= ~PLAYER_FLAGS_IS_MULTI", while the pastebin lacks iterator usage, making me a little confused as to which is supposed to be more up-to-date.

~0016126

Goober5000 (administrator)

The attached patch is more up-to-date. The difference in multi flags is interesting; the first method toggles the flag while the second strictly turns it off. The second method should probably be used because it doesn't depend on the current state of the flag.

~0016131

Goober5000 (administrator)

Committed.
+Notes

+Related Changesets

-Issue History
Date Modified Username Field Change
2014-06-30 09:31 ngld New Issue
2014-06-30 11:45 MjnMixael Note Added: 0015970
2014-06-30 22:08 Goober5000 Note Added: 0015990
2014-06-30 22:08 Goober5000 Status new => code review
2014-06-30 22:08 Goober5000 Target Version => 3.7.2
2014-06-30 23:21 Goober5000 Note Added: 0015993
2014-07-01 02:23 ngld Note Added: 0015999
2014-07-07 02:45 Goober5000 Note Added: 0016033
2014-07-07 09:36 ngld File Added: mainhall_caps.patch
2014-07-07 09:56 ngld Note Added: 0016036
2014-07-20 23:55 niffiwan Note Added: 0016110
2014-07-21 20:58 Goober5000 Note Added: 0016113
2014-07-23 22:04 Goober5000 Note Added: 0016121
2014-07-23 22:40 MageKing17 Note Added: 0016122
2014-07-24 20:30 Goober5000 Note Added: 0016126
2014-07-24 23:33 Goober5000 Changeset attached => fs2open trunk r10939
2014-07-24 23:41 Goober5000 Changeset attached => fs2open trunk r10941
2014-07-24 23:42 Goober5000 Note Added: 0016131
2014-07-24 23:42 Goober5000 Status code review => resolved
2014-07-24 23:42 Goober5000 Resolution open => fixed
2014-07-24 23:42 Goober5000 Fixed in Version => 3.7.2
+Issue History