New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Imperial Stockpile #1607

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Dilvish-fo
Member

Dilvish-fo commented Jun 15, 2017

This PR builds on #1605 and #1606, which I submitted separately to facilitate a more focused discussion of some underlying reorganization of the Production Queue.

Parts of this PR draw heavily on the initial Imperial Stockpile work done by @agrrr3 in #1229 , though as discussed some on our forums I have taken a different path in some areas, and then of course a lot of work was needed on the projections code. There are also a few things that agrrr3 had put into his branch that I haven't adapted here, my main goal was to get the basic Imperial Stockpile working properly with accurate projections, which this does.

  • individual items on the queue by default will NOT draw on the stockpile, they need to be enabled for it via right-click menu. If so enabled, their tooltip will so state, but if not enabled for the stockpile then their tooltip simply does not mention the stockpile. When enabled, there is not currently any specific indicator of just how much PP the item is drawing from the stockpile, but you can easily see that by toggling the stockpile use off and back on to compare.

  • even without enabling any items to draw on the stockpile, any otherwise wasted PPs will be directed to the stockpile (subject to any transfer penalty).

  • The current cap on the amount of resources that can be stored in the stockpile is a modest 10 PP, enough to notice and for testing; it is expected that we'd be adding technologies that could increase that.

  • A readout of the current and projected stockpile status is added to the tooltip for Production Points on the MapWnd menu bar.

  • Per the forum discussion, this initial implementation only penalizes transfers into and out of the stockpile if the source or destination is outside of the resource group containing the empire capital (which is the current site of the stockpile). For such penalized transfers, the penalty is currently very significant (80% penalty). Like for the cap, it is expected that we'd be adding various technologies to improve transfer efficiency.

As noted in #1606 the reorganization of the projections code that this relies upon drastically slows down the projections. So far it has still seemed reasonable to me, but for larger empires or with many AIs this may be a more noticeable problem. If you want to check how long projections are taking you can search your freeorion.log (or an AI log) for the phrase ProductionQueue::Update: Projections took

I am contemplating that I might try implementing a mixed-mode for projections, wherein any resource groups that do not have any items queued with them that are enabled for the imperial stockpile would rely on the tremendously faster dynamic programming approach for projections. Alternatively, I also have some thoughts on how I might possibly be able to pursue a dynamic programming approach even for items enabled for the stockpile, but I am not ready to tackle that just yet.

Streamline ProductionQueue::Update() projections structure
Drops the dynamic programming approach in favor of simply repeatedly
calling SetProdQueueElementSpending().  Projections take substantially more time (by a factor of some 30-40 fold), but the simplified structure is more amenable to supporting implementation of an Imperial Stockpile.

Dilvish-fo added some commits Jun 14, 2017

Introduce Imperial Stockpile empire meters
specifically, METER_IMPERIAL_PP_TRANSFER_EFFICIENCY]] and METER_IMPERIAL_PP_STOCKPILE_LIMIT
(but does not yet implement the actual operation of the stockpile)

draws from https://github.com/agrrr3/freeorion/commits/imperial-supply-stockpile3
Allow build items to be enabled for Stockpile use
Adds various mechanisms and UI elements related to enabling and
disabling a build item's use of the Imperial Stockpile
  --ProductionQueue::Element attribute and serialization
  --ProductionQueueOrder for changing setting
  --ProductionWnd build item display info and context menu entry

** Actual Stockpile use not yet implemented

Draws heavily from draws from https://github.com/agrrr3/freeorion/commits/imperial-supply-stockpile3
Clarify ResourcePool names & comments related to stockpiles
the ResourcePool had previously distinguished resource 'output' (current
turn resource generation) from resource available (output plus stockpile)
but had not exposed the same information for 'output' as it had for 'available'. Some of the existing uses appear to more appropriately correspond to 'output' and so are herein changed.
Implement the core of the Imperial Stockpile
- enables imperial stockpile drawdown by the ProductionQueue, and
takes the stockpile into account for projections.

- currently contributions into, and drawdowns from, the stockpile are made
  without penalty within the same resource group as the empire capital, and
  are subject to a transfer efficiency penalty (initially 80%) for
  transfers to/from different resource groups.

- there is an initial cap of 10 PP for the stockpile

- does not yet implement UI display of current stockpile amount
Add UI display for Imperial Stockpile
displays as part of the MapWnd title bar tooltip for Production Points;
shows current stockpile value, current use from stockpile, and the
expected next turn value for the stockpile
@Dilvish-fo

This comment has been minimized.

Member

Dilvish-fo commented Aug 5, 2017

The underlying basic Production Queue reorg PR has been cleaned up and merged to master, which introduced a couple conflicts, and there were several other conflicts with recent developments in master as well, so I've cleaned all that up and rebased this PR....

@@ -107,6 +112,38 @@ namespace {
return retval;
}
float CalculateNewStockpile(int empire_id, int stockpile_location_id, float starting_stockpile,
const std::map<std::set<int>, float>& available_pp,

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

fix indenting

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Aug 6, 2017

Member

aligned it with in empire_id now.

{
const Empire* empire = GetEmpire(empire_id);
if (!empire) {
ErrorLogger() << "CalculateStockpileContribution() passed null empire. doing nothing.";

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

a null empire wasn't passed; an invalid empire id was passed

}
float stockpile_transfer_efficiency = empire->GetMeter("METER_IMPERIAL_PP_TRANSFER_EFFICIENCY")->Current();
// locally, within same resource group as the stockpile location (empire capital) we currently allow stockpile contribution at no penalty
float local_stockpile_transfer_rate = 1.0;

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

As discussed, remove, or at least disable, the concept of a stockpile location

http://www.freeorion.org/forum/viewtopic.php?p=89155#p89155

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Aug 6, 2017

Member

As discussed, remove, or at least disable, the concept of a stockpile location

Hmm, I hadn't understood that brief exchange to go so far as you are saying now. Stockpile location is in a fair bit of preexisting code; it seems to me that the discussions are still at a bit of an early stage to go through and rip that all out.

Also, I view my role here as more limited, at least currently-- @agrrr3 was working on the imperial stockpile and had on our forums requested help getting the completion projections to work properly; since that was one of my areas of past work, and since it seemed (to me at least) like the imperial stockpile was a good candidate for being a significant part of the Stealth revamp which I understood to be the focus for the next release, I thought I should take on the task of getting a suitable foundation for the imperial stockpile with working projections, which I think I've done. (I'll be happy to still follow up on the various bits of code grooming you've pointed to and any bugfixes.) Overall, though, I am hoping that @agrrr3 will resume activity and run with the baton.

In the meantime, I'd like to suggest we have some more discussion on the forums about the stockpile now that there is a working base for people to play around with.

This comment has been minimized.

@agrrr3

agrrr3 Sep 5, 2017

Contributor

I didnt get as much community feedback as i hoped so i simplified the stockpile code in my branch not to use the stockpile location. Stockpile still has dead code but i will do a cleanup PR after people actually used the stockpile.

float local_stockpile_transfer_rate = 1.0;
float remote_stockpile_transfer_rate = stockpile_transfer_efficiency;
float stockpile_used = boost::accumulate(allocated_stockpile_pp | boost::adaptors::map_values, 0.0f);

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

comment this block of code?

continue;
bool local_to_stockpile = available_group.first.find(stockpile_location_id) != available_group.first.end();
float this_transfer_rate = local_to_stockpile ?
local_stockpile_transfer_rate : remote_stockpile_transfer_rate;

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

weird indenting

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Aug 6, 2017

Member

That was three tabs (12 spaces). I could shorten it to two tabs / 8 spaces (which leaves it 1 space off from aligning with this_transfer_rate, or use 7 spaces to fully align it to this_transfer_rate, though frankly I think those both look more cluttered and less clearly a continuation line than the 12 spaces. Please give me a specific instruction on how you want this indented.

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 6, 2017

Member

4 spaces more indent than the previous line

//DebugLogger() << "queue size: " << queue.size();

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

prefer no spaces at left of blank lines, but (I think?) 6 blank spaces when the indenting is 8 is weird...

// item is not being built at an object that has access to resources, so it can't be produced.
//DebugLogger() << "no resource sharing group for production queue element";
float& group_pp_available = (available_pp_it != available_pp.end()) ?
available_pp_it->second : dummy_pp_source;

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

weird indenting

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Aug 6, 2017

Member

same issue as with the other continuation line-- if you still want it changed please specify the indentation that you want.

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 6, 2017

Member

4 more spaces indent than the previous line

// get max contribution per turn and turns to build at max contribution rate
int location_id = (queue_element.item.CostIsProductionLocationInvariant() ? INVALID_OBJECT_ID : queue_element.location);
int location_id = (queue_element.item.CostIsProductionLocationInvariant() ?
INVALID_OBJECT_ID : queue_element.location);

This comment has been minimized.

@geoffthemedio
float allocation = std::max(0.0f, std::min(element_this_turn_limit, group_pp_available));
// resource sharing group (including any stockpile availability)
float stockpile_conversion_rate = (group.find(stockpile_location_id) != group.end()) ?
local_stockpile_use_rate : remote_stockpile_use_rate;

This comment has been minimized.

@geoffthemedio
@@ -855,6 +920,9 @@ float ProductionQueue::TotalPPsSpent() const {
return retval;
}
/** TODO: Is there any reason to keep this method in addition to the more specific
* information directly available from the empire? This should probably at least be renamed
* to clarify it is non-stockpile output */

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

To me, "Available" specifically means with stockpile considerations included. It's the output, or how much is produced each turn, plus anything additional accessible from a stockpile. Combined, they are the available PP.

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Aug 6, 2017

Member

Keep in mind that this method is returning a ResourceGroup:amount mapping of available PP, not a simple tally, which is provided via ResourcePool::TotalAvailable(). And there we clearly identify TotalAvailable as including stockpile amounts, versus TotalOutput which does not. This current method is really a repackaging of ResourcePool::Available() plus a check for invalid ResourcePool. But also looking at the ResourcePool level I see that this does in fact include the stockpile amount, though in a location-specific fashion. I'll need to look into this a bit more, I suspect I've been misusing this a bit because of that stockpile issue.

Which is making me feel like that is altogether more reason to eliminate this method from here and just rely directly on the underlying ResourcePool::Available() -- extra layers of low-value indirection appear to be ripe for me to make errors with.

In either place, though, there is an issue of how to report the stockpile info. It looks to me like the preexisting code was set up to include the stockpile amount in the resource group that contains the stockpile location. If we move away from a stockpile location then the stockpile amount, whether provided via this or (if we dropped this method) via the underlying ResourcePool::Available() method, would have to be associated with some special set. I have a recollection that the empty set is used for some placeholder purposes in some of the related code and so I think it best not to use that for this. So that leaves the most simple being a set including only Invalid_Object_ID (which is kind of congruent with some of the preexisting stockpile code that basically takes INVALID_OBJECT_ID as the default stockpile location); another possibility instead of Invalid_Object_ID is to designate a new special object ID to be used for stockpile where a location ID is needed. agrrr3 had suggested the value -4 for that purpose, and I can see some appeal to using a new special ID value like -4.

std::map<std::set<int>, float> available_pp = AvailablePP(industry_resource_pool);
int stockpile_location_id = industry_resource_pool->StockpileObjectID();
float available_stockpile = industry_resource_pool->Stockpile();
float stockpile_transfer_efficiency = empire->GetMeter("METER_IMPERIAL_PP_TRANSFER_EFFICIENCY")->Current();

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

should probably rename this meter to clarify what it means... METER_STOCKPILE_INPUT_EFFIC or similar. don't think the imperial meter needs "IMPERIAL" in its name, though...

This comment has been minimized.

@agrrr3

agrrr3 Sep 5, 2017

Contributor

Oh didnt see this comment. The name stems from the idea that the PP are the important thing and not the stockpile. IMPERIAL_PP would be to distinguish from "local" supply group PP.
But i think stockpile is the term that sticked and there is only the imperial one, so it it not necessary to point that out in the name. So I'll follow your suggestion in my branch.

for (int sim_turn = 1; sim_turn <= TOO_MANY_TURNS; sim_turn ++) {
if ((boost::posix_time::ptime(boost::posix_time::microsec_clock::local_time()) -
sim_time_start).total_microseconds()*1e-6 >= TOO_LONG_TIME)

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

weird indenting

for (int sim_turn = 1; sim_turn <= TOO_MANY_TURNS; sim_turn ++) {
if ((boost::posix_time::ptime(boost::posix_time::microsec_clock::local_time()) -
sim_time_start).total_microseconds()*1e-6 >= TOO_LONG_TIME)
break;

This comment has been minimized.

@geoffthemedio

geoffthemedio Aug 5, 2017

Member

after a multi-line if, particularly where the later lines are indented from the first, put { and } around the following indented code

Dilvish-fo added some commits Aug 6, 2017

@Vezzra Vezzra added this to the v0.4.8 milestone Aug 11, 2017

@dbenage-cx dbenage-cx referenced this pull request Sep 5, 2017

Closed

Imperial Stockpile #1710

@Vezzra

This comment has been minimized.

Member

Vezzra commented Sep 15, 2017

@Dilvish-fo, I assume the superseding PR is #1710?

@Vezzra Vezzra modified the milestones: v0.4.8, Gateway to the Void Sep 15, 2017

@Dilvish-fo Dilvish-fo deleted the Dilvish-fo:imperial_stockpile branch Apr 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment