[6160] Asteroids spin like mad

[Dilvish]

They make me a bit ill watching them. Even trying to apply an overall FPS limit of 15 via the options panel doesn't slow them down. reverse merging 6160 fixed the problem for me, though I took at look at the changes in it & can't see why it would have made this problem.

[Geoff the Medio]

Even trying to apply an overall FPS limit of 15 via the options panel doesn't slow them down.

That doesn't really work for limiting FPS in my experience, or at least doesn't work as it seems like it should.

There's an asteroids FPS setting in /default/data/art/planets/planets.xml that should specifically affect the asteroids animation speed.

No idea why changing how the texture were loaded would affect the animation speed though... Maybe the files aren't being ordered the same on your system as mine? When GetAsteroidTextures() in SidePanel.cpp runs, does it return an array of 256 textures?

[Dilvish]

There's an asteroids FPS setting in /default/data/art/planets/planets.xml that should specifically affect the asteroids animation speed.

yes I had checked that, it was fine, I also just tried manually bypassing that value anyways and changing the line to

Code: Select all

m_planet_graphic->SetFPS(15.0);

and then trying FPS to 0.0, but that didn't change anything.
**edit I had goofed in my check of the Play() line, it did indeed stop the graphic.
Even commenting out the Play() line [code//]m_planet_graphic->Play();[/code]didn't do anything to stop or slow the graphic.

No idea why changing how the texture were loaded would affect the animation speed though... Maybe the files aren't being ordered the same on your system as mine? When GetAsteroidTextures() in SidePanel.cpp runs, does it return an array of 256 textures?

It does return 256 textures, and the file order thing sounds like the best bet; I'll try to follow up more on the file ordering issue, & will post back here once I can check it.

** edited

[Dilvish]

Yes, indeed, the order of textures is the problem; here's the first few in the order returned by the new code:

Code: Select all

asteroids1_174.png
asteroids1_166.png
asteroids1_192.png
asteroids1_019.png
asteroids1_147.png
asteroids1_128.png
asteroids1_104.png
asteroids1_131.png
asteroids1_212.png
...

Having GetAsteroidTextures sort the asteroid textures before returning them fixed the problem; the patch is below. I need to run for a bit now but will try to look into the underlying problem, since fixing it just for asteroid textures is just a partial solution.

Code: Select all

Index: UI/SidePanel.cpp
===================================================================
--- UI/SidePanel.cpp    (revision 6162)
+++ UI/SidePanel.cpp    (working copy)
@@ -731,7 +731,20 @@
             retval = ClientUI::GetClientUI()->GetPrefixedTextures(
                 ClientUI::ArtDir() / "planets" / "asteroids", "asteroids1_", false);
         }
-        return retval;
+        std::map<std::string, boost::shared_ptr<GG::Texture> > texture_map;
+        std::vector<std::string> filename_vec;
+        //Logger().debugStream() << "Asteroid texture files";
+        for (std::vector<boost::shared_ptr<GG::Texture> >::iterator it = retval.begin(); it!=retval.end(); it++) {
+            filename_vec.push_back((*it)->Filename());
+            texture_map[(*it)->Filename()] = (*it);
+            //Logger().debugStream() << (*it)->Filename();
+        }
+        std::sort(filename_vec.begin(), filename_vec.end());
+        static std::vector<boost::shared_ptr<GG::Texture> > retval2;
+        for (std::vector<std::string>::iterator it = filename_vec.begin(); it!= filename_vec.end(); it++)
+            retval2.push_back(texture_map[*it]);
+        retval.clear();
+        return retval2;
     }
 
     const std::string EMPTY_STRING;
@@ -1025,6 +1038,7 @@
         const std::vector<boost::shared_ptr<GG::Texture> >& textures = GetAsteroidTextures();
         if (textures.empty())
             return;
+        Logger().debugStream() << "SidePanel::PlanetPanel::CheckDisplayPlanets Asteroid texttures size: " << textures.size();
         GG::X texture_width = textures[0]->DefaultWidth();
         GG::Y texture_height = textures[0]->DefaultHeight();
         GG::Pt planet_image_pos(GG::X(MaxPlanetDiameter() / 2 - texture_width / 2 + 3), GG::Y0);

[Geoff the Medio]

Code: Select all

std::map<std::string, boost::shared_ptr<GG::Texture> > texture_map;

An std::map should already be sorted by increasing key value; it shouldn't be necessary to store the keys elsewhere and sort them.

[Dilvish]

An std::map should already be sorted by increasing key value; it shouldn't be necessary to store the keys elsewhere and sort them.

I rely on that in some cases where it's not really critical, like with ordering the parts in the DesignWnd, but it's weak ordering, right -- not guaranteed? I wanted to be sure they were properly sorted here.

[Dilvish]

The underlying problem was that the boost directory_iterator has no guaranteed order of iteration. I put sorting directly into ClientUI.cpp, so the above changes to SidePanel are unnecessary.

[Geoff the Medio]

...it's weak ordering, right -- not guaranteed? I wanted to be sure they were properly sorted here.

"strict weak ordering" has a particular mathematical definition, which is not "it might be mixed up sometimes", if that's what you mean... For file names as strings, they are all unique, and have a well-defined ordering with built-in comparison operators. If you were able to sort them in a vector, then they'd be auto-sorted in a map as well.

Code: Select all

+bool FNComp(const boost::shared_ptr<GG::Texture> t1, const boost::shared_ptr<GG::Texture> t2) {
+    return t1->Filename() < t2->Filename();
+}

My only concern with that is the lack of parameter validation; if t1 or t2 are empty / null / reset pointers, then the dereferencing with -> might fail. You could just add "t1 && t2 &&" between "return" and "t1->". Otherwise that looks fine.

[Dilvish]

You could just add "t1 && t2 &&" between "return" and "t1->". Otherwise that looks fine.

ok, done & committed.

[Dilvish]

It's kind of funny -- after this patch my fleet icons changed a little (which size-type icon was used for which size fleet). I thought it was a bug and spent a little while looking at it, and finally realized that it was just working correctly now & had been wrong all the time previous

[Geoff the Medio]

That makes sense now... Naglium's video had weird icons, and I wondered why. I assumed it was related to the .9 or .99 at the end of all the stat numbers, but apparently not. Do you get that issue as well, incidentally?

[Dilvish]

...I assumed it was related to the .9 or .99 at the end of all the stat numbers, but apparently not. Do you get that issue as well, incidentally?

No, I don't get that issue.

[Dilvish]

that must be some kind of precision issue he's hitting; I'll bet that the following change to DoubleToString would clear that up

Code: Select all

    double mag = std::abs(val);
+    if (static_cast<int>(mag) < static_cast<int>(mag + 1e-6))
+        mag += 1e-6;

Is it ok if I go ahead and commit that?

[Geoff the Medio]

that must be some kind of precision issue he's hitting; I'll bet that the following change to DoubleToString would clear that up

Code: Select all

    double mag = std::abs(val);
+    if (static_cast<int>(mag) < static_cast<int>(mag + 1e-6))
+        mag += 1e-6;

Is it ok if I go ahead and commit that?

There should be a comment explaining it, and I'd rather wait for him or someone else with the same issue to test if it actually fixes it.

[Nagilum]

Actually that bug is no longer there.
At least with the gcc4.8 build of rev 6142.
I think haven't seen that bug for about a month or more.