Add option for persistent previous maps by stickz · Pull Request #550 · alliedmodders/sourcemod

stickz · 2016-10-10T03:56:27Z

This commit adds an option to the mapchooser allowing for previous maps to be stored persistently. In the event of plug-in termination, it will remember what the previous maps played were. And recall them like nothing happened, if the user has the corresponding ConVar enabled.

It solves the annoyance of having to start from scratch with previous excluded maps, when something like a server reboot or crash happens; or even if the plug-in is updated/reloaded for whatever reason.

KyleSanderson

Definitely needs some work. The feature is good though, I've been running something very similar for a couple years now.

KyleSanderson · 2016-10-14T20:09:17Z

plugins/mapchooser.sp

+
+    	char map[64];
+
+  	while (!file.EndOfFile() && file.ReadLine(map, sizeof(map)))


EndOfFile is undefined if you haven't read a line.

I just put a safety feature in there, to ensure the loop terminates when the last file line is reached. There isn't any errors while running; but that can be removed, if it's not required.

KyleSanderson · 2016-10-14T20:09:40Z

plugins/mapchooser.sp

+    	char path[PLATFORM_MAX_PATH];
+    	BuildPath(Path_SM, path, PLATFORM_MAX_PATH, MAPCHOOSER_TXT);
+
+   	DeleteFile(path);


If you're opening in write mode, as you're doing below, you don't need to delete the existing file.

Alright I'll remove that.

KyleSanderson · 2016-10-14T20:10:04Z

plugins/mapchooser.sp

+    	for (int idx = 0; idx < g_OldMapList.Length; idx++)
+    	{
+        	g_OldMapList.GetString(idx, lastMap, sizeof(lastMap));
+


Why is there so much spacing?

Fixed. The code is bunched closer together, so it's harder to read now.

KyleSanderson · 2016-10-14T20:11:00Z

plugins/mapchooser.sp

+    	if (!FileExists(path))
+    	{      
+    	    	File file = OpenFile(path, "w");
+		file.Close();   


What is going on here? FileExist should be used instead of creating empty files, that you later remove and re-add.

This function creates the text file, if it's not existent OnPluginStart(), to avoid read/write errors.

KyleSanderson · 2016-10-14T20:11:19Z

plugins/mapchooser.sp

 		g_winCount[i] = 0;	
-	}
-
+	}	


What happened here?

I think an unneeded space got removed.

You removed the new line but kept the tab after the closing brace. Could just leave it as it was 😉

peace-maker

You should try to think of better commit messages ;)

peace-maker · 2016-10-14T23:09:05Z

plugins/mapchooser.sp

 #define VOTE_EXTEND "##extend##"
 #define VOTE_DONTCHANGE "##dontchange##"

+#define MAPCHOOSER_TXT "data/mapchooser.txt"


I think maphistory.txt would be a better name than just mapchooser.txt

I chose mapchooser.txt because it's less likely to conflict with other plugins.

mapchooser_history.txt ?

peace-maker · 2016-10-14T23:09:40Z

plugins/mapchooser.sp

 	g_Cvar_ExtendRoundStep = CreateConVar("sm_extendmap_roundstep", "5", "Specifies how many more rounds each extension makes", _, true, 1.0);
 	g_Cvar_ExtendFragStep = CreateConVar("sm_extendmap_fragstep", "10", "Specifies how many more frags are allowed when map is extended.", _, true, 5.0);	
 	g_Cvar_ExcludeMaps = CreateConVar("sm_mapvote_exclude", "5", "Specifies how many past maps to exclude from the vote.", _, true, 0.0);
+	g_Cvar_PersistentMaps = CreateConVar("sm_mapvote_persisentmaps", "0", "Specifies  whether or not previous maps should be persistently stored.", _, true, 0.0, true, 1.0);


Nit: two spaces after Specifies

peace-maker · 2016-10-14T23:11:33Z

plugins/mapchooser.sp

 		g_winCount[i] = 0;	
-	}
-
+	}	


You removed the new line but kept the tab after the closing brace. Could just leave it as it was 😉

peace-maker · 2016-10-14T23:15:25Z

plugins/mapchooser.sp

+    	char path[PLATFORM_MAX_PATH];
+    	BuildPath(Path_SM, path, PLATFORM_MAX_PATH, MAPCHOOSER_TXT);
+
+    	File file = OpenFile(path, "r");


You should add a null check here, if the file can't be opened and return. That lets you get rid of that CreatePreviousMapsTextFile function too!

peace-maker · 2016-10-14T23:16:50Z

plugins/mapchooser.sp

+    	char path[PLATFORM_MAX_PATH];
+    	BuildPath(Path_SM, path, PLATFORM_MAX_PATH, MAPCHOOSER_TXT);
+
+    	File file = OpenFile(path, "w");


Add null check here too. User could always be lacking write permissions in the data folder or for that file specifically.

Done. I told the plugin to log an error as well, so the user knows something is wrong.

KyleSanderson · 2016-10-15T18:20:37Z

plugins/mapchooser.sp

+
+	if (file == null)
+	{
+		file.Close();


If the file is null you don't need to close it...

KyleSanderson · 2016-10-15T18:21:28Z

plugins/mapchooser.sp

+	g_OldMapList.Clear();
+	char map[64];
+
+	while (!file.EndOfFile() && file.ReadLine(map, sizeof(map)))


This needs to be a do while. You cannot call EndOfFile before reading a line, this is present in almost every libc.

KyleSanderson · 2016-10-15T18:21:52Z

plugins/mapchooser.sp

+	if (file == null)
+	{
+		LogError("[SM] Error writing to %s. Check file permissions.", MAPCHOOSER_TXT);
+		file.Close();


Ditto on calling CloseHandle against an INVALID_HANDLE: why?

KyleSanderson · 2016-10-15T18:23:42Z

plugins/mapchooser.sp

 	}
+
+	/* Recall previous maps from a text file, if persistentcy is enabled */
+	if (g_FirstConfigExec)


can be a static.

schrodingerballs · 2016-10-24T10:45:05Z

plugins/mapchooser.sp

 	g_Cvar_ExtendRoundStep = CreateConVar("sm_extendmap_roundstep", "5", "Specifies how many more rounds each extension makes", _, true, 1.0);
 	g_Cvar_ExtendFragStep = CreateConVar("sm_extendmap_fragstep", "10", "Specifies how many more frags are allowed when map is extended.", _, true, 5.0);	
 	g_Cvar_ExcludeMaps = CreateConVar("sm_mapvote_exclude", "5", "Specifies how many past maps to exclude from the vote.", _, true, 0.0);
+	g_Cvar_PersistentMaps = CreateConVar("sm_mapvote_persisentmaps", "0", "Specifies whether or not previous maps should be stored persistently.", _, true, 0.0, true, 1.0);


You could learn how to spell things

stickz, 2016

schrodingerballs · 2016-10-24T10:46:08Z

plugins/mapchooser.sp

 		}
 	}
+
+	/* Recall previous maps from a text file, if persistentcy is enabled */


You could learn how to spell things

stickz, 2016

schrodingerballs · 2016-10-24T10:46:22Z

plugins/mapchooser.sp

 	return;
 }
+
+/* Persisently store previous maps incase user wants to recall them */


You could learn how to spell things

stickz, 2016

peace-maker · 2016-10-25T00:52:40Z

plugins/mapchooser.sp

 		g_OldMapList.Erase(0);
 	}	
+
+	WritePreviousMapsToText();


You might only want to write/create the file if the convar is set to 1. Otherwise people might get error spam from a disabled feature.

I'll compensate by hiding the error if the feature is disabled. It's best to write to text regardless; otherwise, if something happens between ConVar change and map end, it won't save.

If the feature is disabled, it shouldn't change the file. Maybe some admin doesn't want a map to appear in the history and disables that feature for that map?

See PR alliedmodders#550 for more details. This pull request closes the other one.

stickz added 4 commits October 9, 2016 23:50

Add option for persistent previous maps

c747267

Add option for persistent previous maps

ac46d97

Add option for persistent previous maps

401b2fd

Add option for persistent previous maps

1a563d0

KyleSanderson requested changes Oct 14, 2016

View reviewed changes

Add option for persistent previous maps

852e408

peace-maker reviewed Oct 14, 2016

View reviewed changes

stickz added 2 commits October 14, 2016 20:05

Add option for persistent previous maps

e559e08

Add option for persistent previous maps

8e276e3

KyleSanderson requested changes Oct 15, 2016

View reviewed changes

stickz added 7 commits October 21, 2016 16:42

Add option for persistent previous maps

b5be380

Add option for persistent previous maps

7e29077

Add option for persistent previous maps

1c5191f

Add option for persistent previous maps

1978ab6

Add option for persistent previous maps

28bea83

Add option for persistent previous maps

543ab3f

Add option for persistent previous maps

6645f93

schrodingerballs reviewed Oct 24, 2016

View reviewed changes

schrodingerballs syas stickz slhuod lrean how to slepl tgnhis.

2450b93

peace-maker reviewed Oct 25, 2016

View reviewed changes

Add option for persistent previous maps

e5ae2d6

stickz added a commit to stickz/sourcemod that referenced this pull request Sep 22, 2018

Add option for persistent previous maps #2

e965a69

See PR alliedmodders#550 for more details. This pull request closes the other one.

stickz mentioned this pull request Sep 22, 2018

Add option for persistent previous maps #2 #884

Closed

stickz closed this Sep 22, 2018


		char map[64];

		while (!file.EndOfFile() && file.ReadLine(map, sizeof(map)))

Uh oh!

Conversation

stickz commented Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KyleSanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickz Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickz Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peace-maker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schrodingerballs Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickz Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

stickz commented Oct 10, 2016 •

edited

Loading

stickz Oct 14, 2016 •

edited

Loading

stickz Oct 14, 2016 •

edited

Loading

schrodingerballs Oct 24, 2016 •

edited

Loading

stickz Oct 25, 2016 •

edited

Loading