Merge lp:~qcumber-some/widelands/animations into lp:widelands

[Jens Beyer]

I'm feeling I should add a review request to further discuss this branch, instead of discussing it in the bug.
This is about bug #535806 and is an attempt to use on-demand loading of the animations, instead of preloading them upon game start.

Please note that this is not a 'merge request' per se, but a review request on the technology used and the possible outcomes. The branch in its current state is not up for merge yet.

[Gabriel Margiani (gamag)]

I tested your branch and I am very impressed by the short loading time it takes.
If the world animations would be preloaded and only the tribes "on the fly", it
seems to me to be a very good solution for the loading-animations-time problem!

[Jens Beyer (qcumber-some)]

> I tested your branch and I am very impressed by the short loading time it
> takes.
> If the world animations would be preloaded and only the tribes "on the fly",
> it
> seems to me to be a very good solution for the loading-animations-time
> problem!

This would probably not be so easy since the animations are not "tagged" and there is no way to find out which animation is which.

On the other hand, why do you suggest preloading the world? From my point of view, this would not be necessary, this is fast enough right now.

[Gabriel Margiani (gamag)]

After autosave is initialized and the HQ is created, it takes, on my machine, approx. 3 sec to show the game. That's not much, but a little confusing because you heard already the sound of the new HQ Massage. Seems a little like windows booting :)
Because of that I thought of preloading the world.
But your right, I didn't thought of how to implement this and it could be a problem to separate world and tribe animations. So 3 sec. aren't worse enough to make loading more complex.

[Tino (tino79)]

I like it very much, loading times are and feel much reduced on windows.
I've uploaded a win32 build with these changes:
http://widelands.8-schuss.de/Widelands-bzr6373-nomusic-ondemandload-win32.exe

[Jens Beyer (qcumber-some)]

The animations which are loading after creating a map are all animations which are visible, except water animations. For example: Trees, animals, and the visible starting buildings (HQ, road and flag). Nothing else.
I'm currently trying to find a way (more theoretically) how to achieve a preloading of the visible animations before showing them, but I've not yet been successful ;-)

[Chuck Wilder (chuckw20)]

@Tino - Thanks as always for the Windows build. I started an upgrade from Ubuntu 11.10 to 12.04 LTS before testing a linux build of Jens' branch. That was yesterday afternoon. The upgrade is STILL running. :-| Sorry to go slightly off-topic. I'm just looking for a shoulder to cry on.

Thanks again. Now I can at least see for myself what everyone is talking about. :)

Re: tagging the graphics files - If it weren't for the initiative to pursue stacked animations, I would be happy to start tagging the graphics files. It makes a lot of sense from a purely organizational aspect.

[Jens Beyer (qcumber-some)]

Sorry this was not merged but I've unfortunately overwriten my branch with trunk.

I'm setting this back to Review requested once I recreated the changes.

[Jens Beyer (qcumber-some)]

It's up to date again. Sorry for the inconvenience...

[Jens Beyer (qcumber-some)]

I did some more coding, and made the system load the files at the beginning, but letting the Main thread render the animations. This revealed (at least for my system) that the slow part of this task is not reading the files from disk, but the rendering task itself.

Unfortunately, with the current architecture, there is no alternative to rendering the images in the main thread, as OpenGL is not thread-safe and you should not switch the OpenGL context between threads.

I don't know about the Windows experience. But if someone could test the following, this could be confirmed.

* Download one of Tino's patched executables.
* Switch off Antivirus protection.
* Start game, load a savegame with lots of things happening, play a bit, observe 'hanging' when you scroll quickly to parts of the map where new animations need to be loaded.
* Quit the game, load the same savegame.

If you still experience 'hangs' at the same points like before, this would be a good sign that the rendering time is the culprit, not the file loading time itself.

[Nasenbaer (nasenbaer)]

I merged your branch in rev.6426 into trunk. Thank you Jens for this improvement :)!!

=== modified file 'src/graphic/graphic.cc'
--- src/graphic/graphic.cc	2012-03-09 16:57:20 +0000
+++ src/graphic/graphic.cc	2012-06-30 20:10:27 +0000
@@ -1066,32 +1066,25 @@
 void Graphic::load_animations(UI::ProgressWindow & loader_ui) {
 	assert(m_animations.empty());
 
-	clock_t start = clock();
-
-	const std::string step_description = _("Loading animations: %d%% complete");
-	uint32_t last_shown = 100;
 	const uint32_t nr_animations = g_anim.get_nranimations();
-	for (uint32_t id = 0; id < nr_animations;) {
-		const uint32_t percent = 100 * id / nr_animations;
-		if (percent != last_shown) {
-			last_shown = percent;
-			loader_ui.stepf(step_description.c_str(), percent);
-		}
-		++id;
-		m_animations.push_back(new AnimationGfx(g_anim.get_animation(id)));
-	}
-	loader_ui.step(std::string());
+	m_animations.reserve(nr_animations);
+}
 
-	clock_t end = clock();
-	printf
-		("load_animations took %f seconds\n",
-		 (float(end - start) / CLOCKS_PER_SEC));
+void Graphic::ensure_animation_loaded(uint32_t const anim) {
+	if (anim >= m_animations.size()) {
+		m_animations.resize(anim + 1);
+	}
+	if (!m_animations.at(anim - 1))
+	{
+	  log("Loading animation %i\n", anim);
+	  m_animations.at(anim - 1) = new AnimationGfx(g_anim.get_animation(anim));
+	}
 }
 
 /**
  * Return the number of frames in this animation
  */
-AnimationGfx::Index Graphic::nr_frames(const uint32_t anim) const
+AnimationGfx::Index Graphic::nr_frames(const uint32_t anim)
 {
 	return get_animation(anim)->nr_frames();
 }
@@ -1100,7 +1093,7 @@
  * writes the size of an animation frame to w and h
 */
 void Graphic::get_animation_size
-	(uint32_t const anim, uint32_t const time, uint32_t & w, uint32_t & h) const
+	(uint32_t const anim, uint32_t const time, uint32_t & w, uint32_t & h)
 {
 	AnimationData const * const data = g_anim.get_animation(anim);
 	AnimationGfx  const * const gfx  =        get_animation(anim);
@@ -1156,11 +1149,12 @@
  * @param anim the number of the animation
  * @return the AnimationGfs object of the given number
  */
-AnimationGfx * Graphic::get_animation(uint32_t const anim) const
+AnimationGfx * Graphic::get_animation(uint32_t const anim)
 {
-	if (!anim || anim > m_animations.size())
+	if (!anim)
 		return 0;
 
+	ensure_animation_loaded(anim);
 	return m_animations[anim - 1];
 }
 

=== modified file 'src/graphic/graphic.h'
--- src/graphic/graphic.h	2012-02-21 13:42:13 +0000
+++ src/graphic/graphic.h	2012-06-30 20:10:27 +0000
@@ -178,18 +178,19 @@
 	void reset_texture_animation_reminder();
 
 	void load_animations(UI::ProgressWindow & loader_ui);
-	AnimationGfx::Index nr_frames(uint32_t const anim = 0) const;
+	void ensure_animation_loaded(uint32_t anim);
+	AnimationGfx::Index nr_frames(uint32_t const anim = 0);
 	uint32_t get_animation_frametime(uint32_t anim) const;
 	void get_animation_size
 		(const uint32_t anim,
 		 const uint32_t time,
 		 uint32_t & w,
 		 uint32_t & h)
-		const;
+		;
 
 	void screenshot(const char & fname) const;
 	Texture * get_maptexture_data(uint32_t id);
-	AnimationGfx * get_animation(uint32_t) const;
+	AnimationGfx * get_animation(uint32_t);
 
 	void set_world(std::string);
 	PictureID get_road_texture(int32_t roadtex);