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

Unused RP are automatically allocated to (unpaused) researchable techs #1472

Merged
merged 3 commits into from May 4, 2017

Conversation

Projects
None yet
3 participants
@geoffthemedio
Member

geoffthemedio commented Apr 8, 2017

No description provided.

@geoffthemedio geoffthemedio added this to the post v0.4.7 milestone Apr 8, 2017

@@ -166,6 +167,11 @@ namespace {
elem.allocated_rp = 0.0f;
}
}
if (total_RPs_spent >= RPs - EPSILON)
return; // don't further allocate if nothing left to spend

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Apr 8, 2017

Member

The place these lines are located, after the end of the loop, it seems they are only avoiding the logging line that reports how much RP was spent, not actually stopping any allocation the way the comment indicates is intended.

If I move these lines up into the bottom of the loop so that they stop further allocation, then they cause a UI error: start a new game, queue up a cheap tech like Planet Eco and then an expensive one like Radar. Now shift Radar to first spot so that it soaks up all the available RP. Planet Eco still shows as if it is getting RP allocated to it since the loop exited as soon as Radar used up the RP. (The server prevents any actual overallocation during turn processing though.)

This comment has been minimized.

@geoffthemedio

geoffthemedio Apr 8, 2017

Member

This early exit code is now pointless. It's left over from when I had the auto-allocation in SetTechQueueElementSpending, but it's been moved to CheckResearchProgress.

@geoffthemedio geoffthemedio changed the title from Unused RP are automaticallyallocated to (unpaused) researchable techs to Unused RP are automatically allocated to (unpaused) researchable techs Apr 8, 2017

@Dilvish-fo

This comment has been minimized.

Member

Dilvish-fo commented Apr 8, 2017

My first thought was that the idea seems fine/good to prevent folks, especially beginners, from losing out on RP, though a SitRep would be most helpful, both to bring the issue to the attention of the player and so that they could know which tech actually got the RP without having to add a bunch of techs to the queue to see which one had gotten some RP.

But, then I considered that we are talking about adding tech lines where researching one tech might lock you out of others, or at least make them extra expensive. It would not be good to have this auto-allocation trigger any such result. I imagine we could try adding ways to prevent that, but I would be concerned about their reliability.

@geoffthemedio

This comment has been minimized.

Member

geoffthemedio commented Apr 8, 2017

If such tech lines are added, this could be revisited, or restrictions could be added.

(I think there are better ways to make players choose between content options, however that's a forum discussion.)

Lack of a display of progress for techs not currently on the queue is arguably a separate issue.

I'm not so concerned about showing what techs are getting allocations automatically though, as if players care enough to know, they can just enqueue some techs.

geoffthemedio added some commits Mar 18, 2017

-Unused RP are allocated to (unpaused) researchable techs.
-Added functions for logging of research queue.
-Removed unused UnderfundedProject functions.
@geoffthemedio

This comment has been minimized.

Member

geoffthemedio commented May 2, 2017

Objections to merging as is?

@Vezzra

This comment has been minimized.

Member

Vezzra commented May 2, 2017

As far as I'm concerned, go ahead.

@Dilvish-fo

This comment has been minimized.

Member

Dilvish-fo commented May 4, 2017

Sounds fine to me.

@geoffthemedio geoffthemedio merged commit 39c4fba into master May 4, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@geoffthemedio geoffthemedio deleted the AutoAllocateRP branch May 4, 2017

@Vezzra Vezzra added the status:merged label May 4, 2017

wheals added a commit to wheals/freeorion that referenced this pull request Nov 6, 2017

wheals added a commit to wheals/freeorion that referenced this pull request Nov 7, 2017

wheals added a commit to wheals/freeorion that referenced this pull request Nov 7, 2017

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