Add warning for excess stockpile contributions

[Dilvish-fo]

• if adding to stockpile and would put it over ten turns max use

forum discussion

Addresses #2175

The yellowed warning icons could use further improvement, if anybody wants to work on them that would be great.

And it could be that they should just be added as a whole new button rather than just having the single industry-wasted icon shuffles its textures around depending on the current state of waste & stockpile.

[Oberlus]

Tested. Works as you describe. To be explicit:

• Red warning icon appears when contribution of current turn is greater than 10x max use.
• Yellow warning icon appears when contribution of current turn will put total stockpile over 10x max use.

I'd like a different behaviour:

• Red warning icon appears when contribution of current turn will put total stockpile over 10x max use.
• Yellow warning icon appears when contribution of current turn will put total stockpile over 3x max use.

Thus I'd have an earlier warning that my queue is getting empty, after 2 or 3 turns instead of 9 or 10.

Also, a less bright yellow (or pale orange) could work better with the white of the admiration symbol.

[Dilvish-fo]

Also, a less bright yellow (or pale orange) could work better with the white of the admiration symbol.

As I noted above, I would appreciate if anyone else wanted to work on improving those yellow icons, so please make a go of it. I don't plan on putting more time in on them myself.

I'd like a different behavior:

Since we've already started the design discussion in a forum thread (which is generally where we prefer them), I responded there.

@Vezzra since you had some interest in this feature, I think, if you have a chance to consider your preferences please chime in, both on the choice of basic functionality (the original request versus the new requested one, and versus a new tweak I just proposed in the forum thread), and re whether the yellow icons are at least sufficient for 0.4.8.

[Vezzra]

@Dilvish-fo, I posted a reply on the forum.

re whether the yellow icons are at least sufficient for 0.4.8.

They Look fine to me.

[agrrr3]

I think the PR in its current form is sufficient for inclusion. So i'd personally would have this rather merged than not. If someone does a better design/implementation it could be done in another PR.

[Dilvish-fo]

I've revised the triggers:

    // red "waste" icon if the non-project transfer to IS is more than either 3x per-turn use or 80% total output
    // else yellow icon if the non-project transfer to IS is more than 20% total output, or if there is any transfer
    // to IS and the IS is expected to be above 10x per-turn use.

[Dilvish-fo]

Does anyone see an actual error reported in that Travis fail report? All I see is the typical warnings, but I thought it was usually more apparent if it simply failed due to timeout.

I will note that I do see a warning somewhat related to this PR, but I don't think it's actually anything new from this PR:

/freeorion/UI/ResourceBrowseWnd.h: In constructor 'ResourceBrowseWnd::ResourceBrowseWnd(const string&, const string&, float, float, float, bool, float, float, float, bool, float)':
/freeorion/UI/ResourceBrowseWnd.h:50:12: warning: 'ResourceBrowseWnd::m_offset' will be initialized after [-Wreorder]
     GG::Pt                      m_offset;
            ^
/freeorion/UI/ResourceBrowseWnd.h:48:35: warning:   'bool ResourceBrowseWnd::m_show_stockpile_limit' [-Wreorder]
     bool                        m_show_stockpile_limit = false;
                                   ^
/freeorion/UI/ResourceBrowseWnd.cpp:17:1: warning:   when initialized here [-Wreorder]
 ResourceBrowseWnd::ResourceBrowseWnd(const std::string& title_text,

[Oberlus]

If the reordered initialisations do not depend on any of the other initialisations, then you can ignore the warning (or better reorder the initialisations to match the declaration order, or vice versa, to remove the warning). Otherwise it could be a source of corruption.
Better explanation and example: https://stackoverflow.com/questions/1828037/whats-the-point-of-g-wreorder

[Dilvish-fo]

or better reorder the initialisations to match the declaration order, or vice versa, to remove the warning.

Right, I've done that before to remove such warnings. But given where we are at right now (and given that I'm very pressed for time) I don't really want to add unnecessary changes to this PR. I wasn't meaning to ask any advice on that warning, just thought I should acknowledge that the report noted warnings that were at least tangentially related to this PR (and partly as a small bit of reminder to eventually follow up on that).

Did you notice anything, though, about why this last commit failed the Travis check?

[Oberlus]

Oh, then I apologize for the intromission.

Did you notice anything, though, about why this last commit failed the Travis check?

First off, this was the first time I've looked into a Travis CI report. At a first glance couldn't find a "final" diagnosis in the report (following the Details link, I mean) so I have no real knowledge of why is that build failed.

I'm focusing just on the warning you quoted (that seem the one affected by this PR). I found there is a new private variable m_show_stockpile_limit in class ResourceBrowseWnd (not appearing to me in the changes of this PR, but it is certainly a change not present in master), whose declaration in the .h lists firsts the new variable and then m_offset, but whose initialisation in the .cpp happens last, after m_offset's initialisations, and thus the warning. As you already know, it is irrelevant for code execution and this PR should work for everyone. But maybe you are right to point it out as the possible cause of the Travis CI build failure?

In that case, swap those two initialisations (care with the comma) and the warning should be gone:
https://github.com/Dilvish-fo/freeorion/blob/d3efd5ac0a2f4dfcabdd06f8b86525874f9d8fac/UI/ResourceBrowseWnd.cpp#L50-L51

[Dilvish-fo]

but it is certainly a change not present in master

Well that certainly got me curious, but checking the logs via

$ git log -S m_show_stockpile_limit --reverse

reveals that it is from commit 845e2aa which is in both the release branch and master.

But maybe you are right to point it out as the possible cause of the Travis CI build failure?

I wasn't meaning that, a simple warning does not cause a Travis build failure. But the apparent point of failure is not very illuminating either:

[ 97%] Building CXX object CMakeFiles/freeorion.dir/UI/TechTreeArcs.cpp.o
[ 98%] Building CXX object CMakeFiles/freeorion.dir/UI/TechTreeLayout.cpp.o
[ 98%] Building CXX object CMakeFiles/freeorion.dir/UI/TechTreeWnd.cpp.o
CMakeFiles/freeorion.dir/build.make:1526: recipe for target 'CMakeFiles/freeorion.dir/UI/TechTreeArcs.cpp.o' failed
CMakeFiles/freeorion.dir/build.make:1574: recipe for target 'CMakeFiles/freeorion.dir/UI/TechTreeWnd.cpp.o' failed
make[2]: *** [CMakeFiles/freeorion.dir/UI/TechTreeArcs.cpp.o] Terminated
make[2]: *** [CMakeFiles/freeorion.dir/UI/TechTreeWnd.cpp.o] Terminated
make[1]: *** [CMakeFiles/freeorion.dir/all] Terminated
make: *** [all] Terminated

Travis certainly used to be very explicit when it terminated due to timeout, but I notice that the termination happens within a few seconds of 60 minutes being spent on the cmake command, so perhaps it is just a timeout.

[dbenage-cx]

@Dilvish-fo Build passed on restart

[Dilvish-fo]

I decided there was rather trivial risk of unexpected complications from fixing that initialization order issue, so I went ahead and fixed it.

Build passed on restart

@dbenage-cx thanks for restarting that. Once I had looked up the instructions on where to restart it, but then when I actually have a build failure it seems I never find the restart option where I would expect it to be, I guess I'll have to pull up instructions next time and make sure I actually find it.