Merge lp:~widelands-dev/widelands/bug-1690519-playercolor into lp:widelands

[GunChleoc]

Create playercolor unique_ptr as late as possible.

I am assuming that some of the rashes are due to the changes in playercolor caused by this merge:

https://code.launchpad.net/~widelands-dev/widelands/fh1_width_and_mapobject_messages/+merge/318189

So I'm hoping that this change will help.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2196. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/232399748.
Appveyor build 2031. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1690519_playercolor-2031.

[Klaus Halfmann (klaus-halfmann)]

Will compile this and try the MallocMagic, but his will need some time ....

[Klaus Halfmann (klaus-halfmann)]

Sorry, I tried this with:

MallocGuardEdges=y
MallocStackLoggingNoCompact=y
export MallocScribble=y
export MallocCheckHeapStart=0
export MallocCheckHeapEach=100

but before I was able to open the Mutiplayer settings it already crashes
here:

VM Regions Near 0xfffffffffffffe1e:
--> shared memory 00007fffffe12000-00007fffffe13000 [ 4K] r-x/r-x SM=SHM

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 libsystem_malloc.dylib 0x00007fff8f0c0657 szone_check_all + 3160
1 libsystem_malloc.dylib 0x00007fff8f0c4149 malloc_zone_check + 58
2 libsystem_malloc.dylib 0x00007fff8f0c3ea2 internal_check + 17
3 libsystem_malloc.dylib 0x00007fff8f0b5f3b free + 358
4 widelands 0x000000010fa65c32 EditorDecreaseHeightTool::~EditorDecreaseHeightTool() + 34 (sp_counted_impl.hpp:127)
5 widelands 0x000000010fa65cab boost::detail::sp_counted_base::destroy() + 43 (sp_counted_base_clang.hpp:93)
6 widelands 0x000000010fa65e59 boost::detail::sp_counted_base::weak_release() + 57 (sp_counted_base_clang.hpp:128)
7 widelands 0x000000010fa65df2 boost::detail::sp_counted_base::release() + 66 (sp_counted_base_clang.hpp:115)
8 widelands 0x000000010fa65d8a boost::detail::shared_count::~shared_count() + 42 (shared_count.hpp:467)
9 widelands 0x000000010fa65a25 boost::detail::shared_count::~shared_count() + 21 (shared_count.hpp:471)
10 widelands 0x000000010fa3e229 boost::shared_ptr<boost::re_detail::named_subexpressions>::~shared_ptr() + 25 (shared_ptr.hpp:756)
11 widelands 0x000000010fa3e125 boost::shared_ptr<boost::re_detail::named_subexpressions>::~shared_ptr() + 21 (shared_ptr.hpp:756)

SirVers Idea of nonunique usage of a unique ptr souns reassonalbe, can we trace such cases,
maybe I should dive more into the definition of this unique ptr thingees.

[GunChleoc (gunchleoc)]

Now I'm confused. Why on Earth do the multiplayer settings have anything to do with an editor tool?

[Klaus Halfmann (klaus-halfmann)]

Mhh, I will need to attach a debbuger to track this down and the
next week are packed with stuff we should do, sigh.

[Klaus Halfmann (klaus-halfmann)]

I played this for quite a while now (with rare Malloc checks) and got no crash.

The change looks straight foreward.

So this should go in, anyone else?

Had no time for that really long debug session.

review: Approve (copilles, test, codereview)

[GunChleoc (gunchleoc)]

Let's get this in trunk, since it can't do any harm. Thanks for the review and test :)

@bunnybot merge

=== modified file 'src/graphic/animation.cc'
--- src/graphic/animation.cc	2017-05-13 13:14:29 +0000
+++ src/graphic/animation.cc	2017-05-15 11:41:00 +0000
@@ -72,7 +72,7 @@
 	                            float scale) const override;
 	uint16_t nr_frames() const override;
 	uint32_t frametime() const override;
-	std::unique_ptr<const Image> representative_image(const RGBColor* clr) const override;
+	const Image* representative_image(const RGBColor* clr) const override;
 	const std::string& representative_image_filename() const override;
 	virtual void blit(uint32_t time,
 	                  const Rectf& source_rect,
@@ -219,7 +219,7 @@
 	return frametime_;
 }
 
-std::unique_ptr<const Image> NonPackedAnimation::representative_image(const RGBColor* clr) const {
+const Image* NonPackedAnimation::representative_image(const RGBColor* clr) const {
 	assert(!image_files_.empty());
 	const Image* image = (hasplrclrs_ && clr) ? playercolor_image(*clr, image_files_[0]) :
 	                                            g_gr->images().get(image_files_[0]);
@@ -228,7 +228,7 @@
 	const int h = image->height();
 	Texture* rv = new Texture(w / scale_, h / scale_);
 	rv->blit(Rectf(0, 0, w / scale_, h / scale_), *image, Rectf(0, 0, w, h), 1., BlendMode::Copy);
-	return std::unique_ptr<const Image>(rv);
+	return rv;
 }
 
 // TODO(GunChleoc): This is only here for the font renderers.
@@ -335,7 +335,7 @@
 	const auto hash = std::make_pair(id, clr);
 	if (representative_images_.count(hash) != 1) {
 		representative_images_.insert(std::make_pair(
-		   hash, std::move(g_gr->animations().get_animation(id).representative_image(clr))));
+		   hash, std::unique_ptr<const Image>(std::move(g_gr->animations().get_animation(id).representative_image(clr)))));
 	}
 	return representative_images_.at(hash).get();
 }

=== modified file 'src/graphic/animation.h'
--- src/graphic/animation.h	2017-05-03 07:24:06 +0000
+++ src/graphic/animation.h	2017-05-15 11:41:00 +0000
@@ -80,7 +80,7 @@
 	/// An image of the first frame, blended with the given player color.
 	/// The 'clr' is the player color used for blending - the parameter can be
 	/// 'nullptr', in which case the neutral image will be returned.
-	virtual std::unique_ptr<const Image> representative_image(const RGBColor* clr) const = 0;
+	virtual const Image* representative_image(const RGBColor* clr) const = 0;
 	/// The filename of the image used for the first frame, without player color.
 	virtual const std::string& representative_image_filename() const = 0;
 

=== modified file 'src/graphic/playercolor.cc'
--- src/graphic/playercolor.cc	2017-04-28 06:47:01 +0000
+++ src/graphic/playercolor.cc	2017-05-15 11:41:00 +0000
@@ -48,10 +48,10 @@
 	const Image* color_mask = g_gr->images().get(color_mask_filename);
 	const int w = image->width();
 	const int h = image->height();
-	auto pc_image = std::unique_ptr<Texture>(new Texture(w, h));
+	Texture* pc_image = new Texture(w, h);
 	pc_image->fill_rect(Rectf(0, 0, w, h), RGBAColor(0, 0, 0, 0));
 	pc_image->blit_blended(Rectf(0, 0, w, h), *image, *color_mask, Rectf(0, 0, w, h), clr);
-	g_gr->images().insert(hash, std::move(pc_image));
+	g_gr->images().insert(hash, std::unique_ptr<const Texture>(std::move(pc_image)));
 	assert(g_gr->images().has(hash));
 	return g_gr->images().get(hash);
 }