Merge lp:~miroslavr256/widelands/bug-1550568-restool_undo_crash into lp:widelands

[Miroslav Remák]

The cause of this bug is dereferencing a past-the-end iterator in EditorSetResourcesTool::handle_undo_impl and setting a field's resource index to this undefined value.

In certain situations the 'increase/decrease resources' implementation can skip storing some of the old resource indices and amounts, which the undo implementation does not expect.

[GunChleoc (gunchleoc)]

That fixed it, thanks!

@bunnybot merge

review: Approve

[TiborB (tiborb95)]

So if I understand it properly, even if the resource is not changed on particular field (like setting coal on grass), old (=new) resource is saved somewhere and when doing undo it is re-set (=replacing the resource with the very same resource).

Would it be reasonable to create another virtual resource like kNoChange and when doing undo particular field would be skipped, instead of reseting the value? I have performance in my mind. When I work on big maps it is real pain and editor lags very badly... I dont say that this here is a main culprit of course, but improvements in general are deserved

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 840. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/116045985.
Appveyor build 674. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_miroslavr256_widelands_bug_1550568_restool_undo_crash-674.

[GunChleoc (gunchleoc)]

I think performance improvements are always a good idea. Go for it :)

[Miroslav Remák (miroslavr256)]

@tiborb95

> So if I understand it properly, even if the resource is not changed on particular field (like
> setting coal on grass), old (=new) resource is saved somewhere and when doing undo it is re-set
> (=replacing the resource with the very same resource).

Yes, that's right.

> I have performance in my mind. When I work on big maps it is real pain and editor lags very badly...

I didn't really consider performance implications. My guess would be that the overlay update function is mostly at fault. Anyway, could you try this patch and see how it is performance-wise: http://pastie.org/private/cf2coofqafpffzm2h5ra ? This should skip unnecessary fields and still not crash the game after undoing the action.

[GunChleoc (gunchleoc)]

Miroslav, you're now a member on widelands-dev, so we can work on branches together now :)

Could you please create a new branch + merge request?

=== modified file 'src/editor/tools/editor_decrease_resources_tool.cc'
--- src/editor/tools/editor_decrease_resources_tool.cc	2016-01-16 12:55:14 +0000
+++ src/editor/tools/editor_decrease_resources_tool.cc	2016-03-15 05:03:53 +0000
@@ -50,10 +50,11 @@
 		if (amount < 0)
 			amount = 0;
 
+		args->orgResT.push_back(mr.location().field->get_resources());
+		args->orgRes.push_back(mr.location().field->get_resources_amount());
+
 		if (mr.location().field->get_resources() == args->cur_res &&
 			map->is_resource_valid(world, mr.location(), args->cur_res)) {
-			args->orgResT.push_back(mr.location().field->get_resources());
-			args->orgRes.push_back(mr.location().field->get_resources_amount());
 			map->initialize_resources(mr.location(), args->cur_res, amount);
 		}
 

=== modified file 'src/editor/tools/editor_increase_resources_tool.cc'
--- src/editor/tools/editor_increase_resources_tool.cc	2016-01-14 22:09:24 +0000
+++ src/editor/tools/editor_increase_resources_tool.cc	2016-03-15 05:03:53 +0000
@@ -47,11 +47,12 @@
 		if (amount > max_amount)
 			amount = max_amount;
 
+		args->orgResT.push_back(mr.location().field->get_resources());
+		args->orgRes.push_back(mr.location().field->get_resources_amount());
+
 		if ((mr.location().field->get_resources() == args->cur_res ||
 				!mr.location().field->get_resources_amount()) &&
 				map->is_resource_valid(world, mr.location(), args->cur_res)) {
-			args->orgResT.push_back(mr.location().field->get_resources());
-			args->orgRes.push_back(mr.location().field->get_resources_amount());
 			map->initialize_resources(mr.location(), args->cur_res, amount);
 		}
 	} while (mr.advance(*map));