Closed stevenh closed 10 years ago
k0rd
commented
11 years ago Still, Main.mapUnfinished is never checked. This change does a little more than is described by commit message.
I won't go so far as to say it is an unworking patch, but I'd really like more information as to what you are trying to accomplish by reenabling the async callbacks and such...
[edit] a lot of those conditionals will never return true, and are client code.
stevenh
commented
11 years ago Sorry I didn't respond earlier didn't get a notifaction about your comment for some reason.
Anyway I'm not sure what you mean there by Main.mapUnfinished or conditionals will never return true, I'm guessing you think there's changes which there aren't due to the diff, so I'll explain.
While it doesn't look like it, due to diff being crappy, there's actually not many changes at all.
In a bit more detail. essentially you have:- == In realsaveWorld ==
== In smCallBack ==
The following is a diff performed without changing the indentation, which is much easier to tell what's going on:-
diff --git a/Terraria/WorldGen.cs b/Terraria/WorldGen.cs
index 67ff55c..e9a205b 100644
--- a/Terraria/WorldGen.cs
+++ b/Terraria/WorldGen.cs
@@ -1492,20 +1492,20 @@ namespace Terraria
{
return;
}
+ try
+ {
WorldGen.saveLock = true;
while (WorldGen.hardLock)
{
Main.statusText = Lang.gen[48];
+ Thread.Sleep(10);
}
lock (WorldGen.padlock)
{
- try
+ if (!Directory.Exists(Main.WorldPath))
{
Directory.CreateDirectory(Main.WorldPath);
}
- catch
- {
- }
if (!Main.skipMenu)
{
bool value = Main.dayTime;
@@ -1525,8 +1525,8 @@ namespace Terraria
{
Stopwatch stopwatch = new Stopwatch();
stopwatch.Start();
- string text = Main.worldPathName + ".sav";
- using (FileStream fileStream = new FileStream(text, FileMode.Create))
+ string worldPathNameSave = Main.worldPathName + ".sav";
+ using (FileStream fileStream = new FileStream(worldPathNameSave, FileMode.Cr
{
using (BinaryWriter binaryWriter = new BinaryWriter(fileStream))
{
@@ -1787,17 +1787,24 @@ namespace Terraria
{
Main.statusText = Lang.gen[50];
string destFileName = Main.worldPathName + ".bak";
- File.Copy(Main.worldPathName, destFileName, true);
+ if (File.Exists(destFileName))
+ {
+ File.Delete(destFileName);
}
- File.Copy(text, Main.worldPathName, true);
- File.Delete(text);
+ File.Move(Main.worldPathName, destFileName);
+ }
+ File.Move(worldPathNameSave, Main.worldPathName);
}
}
- WorldGen.saveLock = false;
}
}
}
: }
+ finally
+ {
+ WorldGen.saveLock = false;
+ }
+ }
public static void randMoon()
{
Main.moonType = WorldGen.genRand.Next(Main.maxMoons);
@@ -12061,6 +12068,8 @@ namespace Terraria
{
return;
}
+ try
+ {
WorldGen.hardLock = true;
Main.hardMode = true;
if (WorldGen.genRand == null)
@@ -12122,8 +12131,12 @@ namespace Terraria
{
Netplay.ResetSections();
}
+ }
+ finally
+ {
WorldGen.hardLock = false;
}
+ }
public static void StartHardmode()
{
if (Main.netMode == 1)As you can see from the above the key change is ensuring the two flags, which are reset to false before returning during normal operation, are always reset to false even if there's an error.
This ensures that even after an error, the code will still attempt a save when next requested, which it will currently never do.
Hope that makes things clearer :)
k0rd
commented
11 years ago Hey - sorry, yes, I know what it is like to be too busy to reply.
So, I wasn't looking at the github diff because of how hard they are to read when whitespace changes. I pulled your repo at https://github.com/stevenh/TerrariaAPI-Server and diffed that.
--- WorldGen.cs 2013-10-11 02:26:59.461595124 -0400
+++ /home/k0rd/tmp/stevenh-terrariaapi/TerrariaAPI-Server-master/Terraria/WorldGen.cs 2013-10-11 03:15:41.979593724 -0400
@@ -1100,7 +1100,7 @@
{
ThreadPool.QueueUserWorkItem(new WaitCallback(WorldGen.worldGenCallBack), 1);
}
- /*public static void SaveAndQuitCallBack(object threadContext)
+ public static void SaveAndQuitCallBack(object threadContext)
{
Main.menuMode = 10;
Main.gameMenu = true;
@@ -1224,7 +1224,7 @@
public static void playWorld()
{
ThreadPool.QueueUserWorkItem(new WaitCallback(WorldGen.playWorldCallBack), 1);
- }*/
+ }
public static void saveAndPlayCallBack(object threadContext)
{
WorldGen.saveWorld(false);
@@ -1233,14 +1233,14 @@
{
ThreadPool.QueueUserWorkItem(new WaitCallback(WorldGen.saveAndPlayCallBack), 1);
}
- /*public static void saveToonWhilePlayingCallBack(object threadContext)
+ public static void saveToonWhilePlayingCallBack(object threadContext)
{
Player.SavePlayer(Main.player[Main.myPlayer], Main.playerPathName);
}
public static void saveToonWhilePlaying()
{
ThreadPool.QueueUserWorkItem(new WaitCallback(WorldGen.saveToonWhilePlayingCallBack), 1);
- }*/
+ }
public static void serverLoadWorldCallBack(object threadContext)
{
WorldGen.loadWorld();
@@ -1307,7 +1307,7 @@
}
public static void clearWorld()
{
- /*if (Main.mapReady)
+ if (Main.mapReady)
{
for (int i = 0; i < WorldGen.lastMaxTilesX; i++)
{
@@ -1332,7 +1332,7 @@
Main.mapTime = 0;
Main.updateMap = false;
Main.mapReady = false;
- Main.refreshMap = false;*/
+ Main.refreshMap = false;
WorldGen.spawnHardBoss = 0;
WorldGen.totalSolid2 = 0;
WorldGen.totalGood2 = 0;
@@ -1492,20 +1492,20 @@
{
return;
}
+ try
+ {
WorldGen.saveLock = true;
while (WorldGen.hardLock)
{
Main.statusText = Lang.gen[48];
+ Thread.Sleep(10);
}
lock (WorldGen.padlock)
{
- try
+ if (!Directory.Exists(Main.WorldPath))
{
Directory.CreateDirectory(Main.WorldPath);
}
- catch
- {
- }
if (!Main.skipMenu)
{
bool value = Main.dayTime;
@@ -1525,8 +1525,8 @@
{
Stopwatch stopwatch = new Stopwatch();
stopwatch.Start();
- string text = Main.worldPathName + ".sav";
- using (FileStream fileStream = new FileStream(text, FileMode.Create))
+ string worldPathNameSave = Main.worldPathName + ".sav";
+ using (FileStream fileStream = new FileStream(worldPathNameSave, FileMode.Create))
{
using (BinaryWriter binaryWriter = new BinaryWriter(fileStream))
{
@@ -1787,16 +1787,23 @@
{
Main.statusText = Lang.gen[50];
string destFileName = Main.worldPathName + ".bak";
- File.Copy(Main.worldPathName, destFileName, true);
+ if (File.Exists(destFileName))
+ {
+ File.Delete(destFileName);
+ }
+ File.Move(Main.worldPathName, destFileName);
+ }
+ File.Move(worldPathNameSave, Main.worldPathName);
}
- File.Copy(text, Main.worldPathName, true);
- File.Delete(text);
}
}
- WorldGen.saveLock = false;
}
}
}
+ finally
+ {
+ WorldGen.saveLock = false;
+ }
}
public static void randMoon()
{
@@ -12061,6 +12068,8 @@
{
return;
}
+ try
+ {
WorldGen.hardLock = true;
Main.hardMode = true;
if (WorldGen.genRand == null)
@@ -12122,8 +12131,12 @@
{
Netplay.ResetSections();
}
+ }
+ finally
+ {
WorldGen.hardLock = false;
}
+ }
public static void StartHardmode()
{
if (Main.netMode == 1)
@@ -39063,8 +39076,8 @@
}
public static void SectionTileFrame(int startX, int startY, int endX, int endY)
{
- /*Main.mapTime = Main.mapTimeMax + 10;
- WorldGen.noMapUpdate = true;*/
+ Main.mapTime = Main.mapTimeMax + 10;
+ WorldGen.noMapUpdate = true;
int num = startX * 200;
int num2 = (endX + 1) * 200;
int num3 = startY * 150;
@@ -39347,7 +39360,7 @@
{
return;
}
- /*if (Main.netMode != 2 && Main.mapEnabled && !WorldGen.noMapUpdate && !WorldGen.gen && Main.map[i, j] != null && Main.map[i, j].light > 0)
+ if (Main.netMode != 2 && Main.mapEnabled && !WorldGen.noMapUpdate && !WorldGen.gen && Main.map[i, j] != null && Main.map[i, j].light > 0)
{
Main.map[i, j].setTile(i, j, Main.map[i, j].light);
if (Main.map[i, j].changed())
@@ -39363,7 +39376,7 @@
Main.mapUnfinished = true;
}
}
- }*/
+ }
if (Main.tile[i, j].wall == 0)
{
Main.tile[i, j].wallColor(0);
@@ -40043,7 +40056,7 @@
{
if (i > 5 && j > 5 && i < Main.maxTilesX - 5 && j < Main.maxTilesY - 5 && Main.tile[i, j] != null)
{
- /*if (Main.netMode != 2 && Main.mapEnabled && !WorldGen.noMapUpdate && !WorldGen.gen && Main.map[i, j] != null && Main.map[i, j].light > 0)
+ if (Main.netMode != 2 && Main.mapEnabled && !WorldGen.noMapUpdate && !WorldGen.gen && Main.map[i, j] != null && Main.map[i, j].light > 0)
{
Main.map[i, j].setTile(i, j, Main.map[i, j].light);
if (Main.map[i, j].changed())
@@ -40060,7 +40073,7 @@
Main.mapUnfinished = true;
}
}
- }*/
+ }
if (!Main.tile[i, j].active())
{
Main.tile[i, j].halfBrick(false);
@@ -46490,7 +46503,7 @@
catch
{
}
- /*if (Main.netMode != 2 && Main.mapEnabled && !WorldGen.noMapUpdate && !WorldGen.gen && i > 0 && j > 0 && Main.map[i, j] != null && Main.map[i, j].light > 0)
+ if (Main.netMode != 2 && Main.mapEnabled && !WorldGen.noMapUpdate && !WorldGen.gen && i > 0 && j > 0 && Main.map[i, j] != null && Main.map[i, j].light > 0)
{
Main.map[i, j].setTile(i, j, Main.map[i, j].light);
if (Main.map[i, j].changed() && !flag)
@@ -46504,7 +46517,7 @@
}
Main.mapUnfinished = true;
}
- }*/
+ }
}
}
}I am assuming then, that I am looking at the wrong repo.
stevenh
commented
11 years ago That's the right repo but lots of those changes aren't mine ;-)
Looks like your comparing with code which is one changeset ahead, namely: https://github.com/Deathmax/TerrariaAPI-Server/commit/09e81c13128f5c8341841b6e83b73cf24d85d469
Which would explain all the additional differences.
If you have my repro checked out the following should show you the gist of things: git diff -w -r 240c1e2
k0rd
commented
11 years ago You are absolutely right. I didn't consider the timeframe here.
All of my concerns are alleviated; if nobody else has a concern, I think we should merge this.
Olink
commented
10 years ago Well, I suppose someone will need to redo these changes since it no longer auto-merges. While I think the try/catch code is useful, the copy/move/delete stuff I find iffy (i don't have metrics on timings or disk rate stuff) and the sleep should be fine, although using a semaphore or etc would be better.
If I have time sometime this weekend I can redo these changes.
For example this happens if save fails due to a permission or disk space error.
Optimised the save routine to use File.Delete + 2 x File.Move's instead of 2 x File.Copy's. This reduces the number of bytes written to disks by world size x 2 and eliminates all disk reads in this code path.