News:

Simutrans Wiki Manual
The official on-line manual for Simutrans. Read and contribute.

JIT2 Stability and Power Animation Fix

Started by DrSuperGood, January 11, 2016, 07:23:20 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

DrSuperGood

Do not have much time to describe it but this does the following.

Fixes jitter with industry power demands when working in JIT2 mode.
Reduces power network instability (oscillations) at usage >100% by adding power boost ramp-up (falls instantly, ramps up slowly).
Fixes distribution transformer power animations as well as a small code cleanup with that regard.

The transformer animations problem was the result of a fixed-point overflow error. Pak128 steelmills used so much power that it caused the computation to overflow so the wrong factor was worked out.

Someone may want to check with JIT0/1 games to make sure there is no regression (there should not be any, but just in case).

TurfIt

There's a lot of commented out code in the patch. Can you cleanup?  Hard to tell if it's just leftover debugging, or intended...

DrSuperGood

Turns out more stuff is broken with power than I first thought. On top of some code revisions I am also fixing more of power so a revised patch will take a day or so.

I am trying tackle the following at the moment.
BUG: Income from power does not scale with month length yet maintainance cost of power infrastructure does.
BUG: Overflow in power metering logic causes large consumers (1Gw+) to not pay for most of the electricity they use.
FEATURE: Adjustable energy price. Trying to sort out the logic, a setting might come later.

Dwachs

Quote from: DrSuperGood on January 12, 2016, 07:19:26 AM
BUG: Income from power does not scale with month length yet maintainance cost of power infrastructure does.
Income should not scale with month length. The month length already provides the scaling: the longer the month the  more often income is generated -> correctly scaled. This is the same with income from transport: check vehicle_t and ware_t ::calc_revenue.
Parsley, sage, rosemary, and maggikraut.

DrSuperGood

Quote
Income should not scale with month length. The month length already provides the scaling: the longer the month the  more often income is generated -> correctly scaled. This is the same with income from transport: check vehicle_t and ware_t ::calc_revenue.
Actually income per month should scale up with month length because maintenance scales up with month length. Currently the income per month remains constant no matter month length because the income per tick is scaled down by the amount month length is scaled up. Hence why I think it is a bug.

Or are you trying to say that a double long month runs twice as many ticks per second as well as needing twice as many seconds per month (double scaling so an increase in 2 results in 4 times as many ticks per month)? In that case it is confusing as... and the current implementation would be correct.

Dwachs

The wording in my post above was maybe not as correct as needed: I mean the income of one completed transport or one delivery of power should not be scaled with month length. If month is longer/shorter then more/less transports are completed, so the scaling is implicit.

Quote from: DrSuperGood on January 12, 2016, 04:09:00 PMCurrently the income per month remains constant no matter month length because the income per tick is scaled down by the amount month length is scaled up. Hence why I think it is a bug.
Do you mean the calls to inverse_scale_with_month_length in senke_t::step ? That indeed looks like a bug.
Parsley, sage, rosemary, and maggikraut.

DrSuperGood

Quote
Do you mean the calls to inverse_scale_with_month_length in senke_t::step ? That indeed looks like a bug.
Yes that is the scaling I am referring to. I am guessing it must have been thrown in when developers were still deciding how months were to scale.

However simply removing it is not enough as there are other issues such as overflow. I am also trying to change when transformers pay to rather be based on time (every 10-30 seconds) rather than every 20 dollars accumulation. This is to stop the income message spam that can occur with high energy consumers (once the logic is working of course). Also want to add some constant for the value of power, which can eventually be exposed as a game constant for pakset authors to modify.

TurfIt

I never did understand the scaling there. Too bad the original discussion thread was lost with the Sourceforge dev forum shutdown...

As month length scales up, factory production scales up, power demand scales down. i.e. Power required remains constant across all month lengths.
If there's no income scaling (as in the legacy ==65535 case),  double production, halve power, but double length -> double energy delivered -> double income.  And since maintenance was also doubled, profit margin maintained.
With the inverse scaling, the income is halved. At high bpms, powerline income drops to nothing.

The overflow is IMHO due to pak128 having set unreasonable power demands. 10 times the former default case. Having a steel mill require 2 large nuclear reactors to power it....    IMO the code should remain, there needs to be limits somewhere, and pak128 has crossed them yet again (same as factory storage's before...)



DrSuperGood

#8
Quote
As month length scales up, factory production scales up, power demand scales down. i.e. Power required remains constant across all month lengths.
Power demand remains constant because power is already time invariant.

As month length scales up, factory production scales up so factory energy usage scales up.

One can simply look at it as bits per month determine the length of month in seconds (each shift doubles that length). Maintenance is defined as an amount per unit time (so scales up with month length), production is defined as an amount per unit time (so scales up per unit length) and power income is defined as a value per unit energy, with power defined as energy per unit time.

TurfIt

I'm not sure if you're agreeing or arguing...

Although I was not clear on items that are explicitly scaled in the code vs scale by default along with the more ticks per month.  By power demand scaling down I meant with respect to the production per month of the factory. For a given production of 1000/month, when at a higher bpm, the power demand is lower. But in practice higher bpm also changes the base production/month, so you end up with 2000/month and the same power.

Regardless, I still think scaling the powerline income is wrong.

DrSuperGood

Quote
By power demand scaling down I meant with respect to the production per month of the factory. For a given production of 1000/month, when at a higher bpm, the power demand is lower. But in practice higher bpm also changes the base production/month, so you end up with 2000/month and the same power.
One must remember that the production per month of factories is purely for the UI. Internally they operate in production per tick. As such it is logical that longer months with proportionally more ticks will produce proportionally more. Power is just like production, it is defined in units per tick with the difference being that users are shown the monthly production while second based power usage. One could also convert power into monthly units, with GJ/month being the distribution unit and then that quantity would scale with month length. Ultimately though the user has to be paid for the amount of work done, be it production or energy used and that value per unit does not (well should not, it currently does incorrectly for energy) change with month in the current approach of balancing.

DrSuperGood

Sorry for double post, but I think it is worth posting an update.

While trying to make power networks save correctly (same state after load as during save) I have stumbled across more things that could do with some work.

Power network ID is a memory reference?! I understand it is used internally as a reference but honestly that should not be shown to a user. I can understand that it is likely the result of some hacky debugging to show which leitung_t objects are on the same network but what I really do not understand is why write this memory reference to file for every leitung_t object when saving? After writing this 4 byte garbage value (which is read but never used) it then goes through the effort of merging all leitung_t on load (something done with multithreading) to create new powernet objects. So not only is 4 bytes per leitung_t wasted, but also it then does a demanding and complex merger operation on load which I would imagine should only be done if you were trying to save space per leitung_t object.

uint32 value; // only used in this extract

obj_t::rdwr(file);

if(file->is_saving()) {
value = (unsigned long)get_net(); // typecast from (powernet_t *) to (unsigned long)
file->rdwr_long(value);
}
else {
file->rdwr_long(value);
set_net(NULL);
}

Now there are two possible approaches to deal with this. One could modify the powernet_t into a saved type and then re-purpose these 4 garbage bytes in future releases to be a reference to a specific powernet_t object once loaded. On-load merger would only need to manage the graphics then as all leitung_t objects would already know what powernet_t they belong with from when they were saved. Save size increased by a tiny amount.

The other, and arguably easier, approach is to just stop writing those 4 bytes in future versions. Some hacky tricks are then needed so that the resulting equivalent powernet_t objects have their state restore correctly after load. One could add proper numbering to powernet_t such that every network is given a human friendly id (eg 1, 2, 3 etc) for visual display at a later time. Save size reduced by possibly several hundred bytes.

Currently 1 tick of power is lost on load (save/load error) which I am trying to fix as part of general power improvements. At the moment I am leaning towards the easier solution, as all I need to do is save the next_tick values at the senke_t and pumpe_t objects and feed those back into the produced powernet_t as its computed result. The combined fixes would add 8 bytes per senke_t, 4 bytes per pumpe_t as well as 4 bytes for a piece of general game state (trivial compared with savings).

Isaac Eiland-Hall

(I issued a warning to mess with you - but for clarification - it's not a doublepost because it was more than 24h since your post that you replied to) :)

prissi

Autosaves could have sync_steps ocurring while saving of the map.

If  you want "human friendly"  ids, I would go for a powernet handle type, just like stations (halthandle_t), convois, lines, ... That would give more human friendly ids as a byproduct.

DrSuperGood

New improved patch. A lot of power code was overhauled.

Power income now occurs every fixed interval. All distribution transformers will be paid out at the same time. This time is roughly save invariant so one can expect similar time and values to occur after loading as did after saving. A 1 cent variance was occasionally observed which is quite possibly down to incorrect reproduction of tick steps after loading compared with what occurs after saving.

Fragmenting and merging power networks should be seamless as far as power distribution goes.

You get paid 2 cent for every mega-joule you deliver, invariant of month length.

Code hopefully more robust for large values.

Several easy to change and meaningful constants added. These could be broken out to game setting values at a later time.

prissi

#15
In the original code the supply was constant (as in reality for most power sources). Now it scales with demand. I lost a little overview, but does this mean now at low demand there will be low consumption? Because this would force everyone supplying a power station to build powerlines and would invalidate countless tutorial on how to start a game.

May I suggest a little more descriptive name than srdwr for senke_t, like global_rdwr() or static_rdwr(). That could become then the new standard for all the few others doing that too.

TurfIt

Power demand is changing with the production rate of the industries in JIT2, but the consumption rate of a electricity generating industry does not change with the power supplied. IMHO that should be done at some point, but the patch I'd previously done wasn't liked...

A few comments after a quick read of the patch:

max capacity had comments stating what the limits were rather than just a magic number appearing in code. Are these limits still valid? Then the comments should remain. If not, new comments with the new limits.

add_supply/demand respected max capacity. New modify_* does not. Comments say they do.

How does a sint32 give a value between 0 and 1?  - get_demand_factor() comment.

User friendly net id would be nice - get rid of using a pointer for that display.

+   // usage logic could go here
If if could/should, then where is it?

+/************************************ Distriubtion Transformer Code ********************************************/
(pedantic) distribution transformers are rather below the scope of the Simutrans electric system.

+   const sint32 pay_period = PRODUCTION_DELTA_T * 10; // should be tied to game setting
Why would the pay period need a setting?

+   const sint32 mjpc = (1 << POWER_TO_MW) / 2; // should be tied to game setting
Yes. Please do so.

+   static sint32 payment_counter; // might want to save this for determinisim
Saveformat already changing, why not this too...

// step powerlines - required order: pumpe, senke, then powernet
And then you change it to powernet, pumpe, senke. ? ? ?
Changing this is why you needed to change around/add all those next_, this_, and last_ that weren't there before. Again, if the new order is correct (and for JIT1 as well), then the comments need updating.

What was the animation fix?

DrSuperGood

QuoteIn the original code the supply was constant (as in reality for most power sources). Now it scales with demand. I lost a little overview, but does this mean now at low demand there will be low consumption? Because this would force everyone supplying a power station to build powerlines and would invalidate countless tutorial on how to start a game.
The foundations for such a feature are there, however it still mechanically works like it currently does. Power stations will produce power independently from the power demand they are linked with. The power produced scales with the power station boost, so passengers and mail might cause it to make more or less power. Power stations produce at full rate even if no transformer is connected.

Consumers scale power based on work done. This is a feature of JIT2 that is in the current release and reasonably well accepted, although debate remains to what the maximum power consumption should be. The only difference is the way the work is represented. Currently it is prone to jitter and oscillations. This patch should hopefully fix both those issues.

Quote
May I suggest a little more descriptive name than srdwr for senke_t, like global_rdwr() or static_rdwr(). That could become then the new standard for all the few others doing that too.
was also thinking "static_rdwr" however I thought people wanted to keep those names abbreviated and short for some reason.

Quote
Power demand is changing with the production rate of the industries in JIT2, but the consumption rate of a electricity generating industry does not change with the power supplied. IMHO that should be done at some point, but the patch I'd previously done wasn't liked...
I fully agree, but how it would effect the power stations needs to be decided and discussed.

Quote
max capacity had comments stating what the limits were rather than just a magic number appearing in code. Are these limits still valid? Then the comments should remain. If not, new comments with the new limits.
Mechanically there are still limits due to the fixed point mathematics used. These limits should be slightly larger than before. The 4 TW limit remains because it is a sensible limit (more than the Wikipedia quoted world energy generation amount). I did make it less "magic" though by using the compiler to derive it from human readable values.

Quote
add_supply/demand respected max capacity. New modify_* does not. Comments say they do.
Ops... was a last minute change. I removed the checks since they really are not necessary as overflow should not be possible (you would need billions of industries with insane power demand each). Only reason there is still ultimately a bounds check is so it ends at some nice readable number and does not explode the UI.

Quote
How does a sint32 give a value between 0 and 1?  - get_demand_factor() comment.
Fixed point fractional number. If there was a fixed point type it would use that.

Quote
User friendly net id would be nice - get rid of using a pointer for that display.
Yes they would be nice. Something to do some day, as that change is not really related to the changes in this patch.

Quote
Why would the pay period need a setting?
Why not? Maybe in some paksets they want it longer than others. In any case it is there as a human readable constant for people to modify if they ever decide 10 seconds is not right.

Quote
Yes. Please do so.
This patch is already running into a lot of changes everywhere. Probably a good idea for that to be a separate patch.

Quote
Saveformat already changing, why not this too...
It already is saved. Originally I was not intending for the patch to add the save functionality, with that coming as a separate patch. However after some discussion on one of the server games I decided to move ahead and throw saving in as well so the feature feels more complete. I must have forgoten to remove the comment.

Quote
And then you change it to powernet, pumpe, senke. ? ? ?
Changing this is why you needed to change around/add all those next_, this_, and last_ that weren't there before. Again, if the new order is correct (and for JIT1 as well), then the comments need updating.
The ordering was changed to make powernet reconstruction easier. The substations and distribution transformers store their current tick power when saved. This is then pushed to their powernet on load, and conserved through mergers. Since it advances the powernet first it solves for these loaded values on its first tick. The transformers can then correctly return the solved power on their first tick. Industries save their last computed power amounts so the system is kept constantly fed. The only real difference it makes is that the logical pause occurs before the powernet tick rather than after.

The whole "next_", "this_" and "last_" thing was added in a previous patch. They are needed to preserve the computed amounts between ticks for UI purposes, since you can only show what has been solved. This logic was needed due to the dynamic power consumption of JIT2 which causes it to appear unstable.

Powernets will load and be created empty since previous tick results are not saved or relevant to the power system. Their tick results should still be correct.

The UI shows you what the last tick's results were and not what the results will be for the next tick.
Quote
What was the animation fix?
Distribution transformers flicker when not supplying 100% of power requirements. This logic was prone to overflows when a consumer used more than ~2 Gigawatts of power. One can argue if a consumer should be using so much power but if they did (pak128 steel mill with JIT2 on) then it broke. Same problem occurred with payment logic, which the new metering logic should have fixed.

Ters

Quote from: TurfIt on January 24, 2016, 11:57:05 PM
Power demand is changing with the production rate of the industries in JIT2, but the consumption rate of a electricity generating industry does not change with the power supplied. IMHO that should be done at some point, but the patch I'd previously done wasn't liked...

Not by much, though. It should be possible to play without having to build powerlines. That's not my job as a train company. Possibly something similar to how production is affected on the other end.

TurfIt

Quote from: DrSuperGood on January 25, 2016, 04:19:13 AM
was also thinking "static_rdwr" however I thought people wanted to keep those names abbreviated and short for some reason.
static_ would be good.


Quote from: DrSuperGood on January 25, 2016, 04:19:13 AM
I fully agree, but how it would effect the power stations needs to be decided and discussed.
Previous discussions/patch: Electrical issues
New discussion could continue there...


Quote from: DrSuperGood on January 25, 2016, 04:19:13 AM
Mechanically there are still limits due to the fixed point mathematics used. These limits should be slightly larger than before. The 4 TW limit remains because it is a sensible limit (more than the Wikipedia quoted world energy generation amount). I did make it less "magic" though by using the compiler to derive it from human readable values.
Having the limits explicitly mention in the code would be good as they were before. Far too often someone changes something and overflows a fixed point calc.


Quote from: DrSuperGood on January 25, 2016, 04:19:13 AM
Fixed point fractional number. If there was a fixed point type it would use that.
Fixed point doesn't magically allow a value between 0 and 1 for an integer. I'm sure you mean between 0 and the scaling factor - so say that.


Quote from: DrSuperGood on January 25, 2016, 04:19:13 AM
Why not? Maybe in some paksets they want it longer than others. In any case it is there as a human readable constant for people to modify if they ever decide 10 seconds is not right.
Minimize the already borderline excessive number of parameters in simuconf.tab... It just doesn't seem like something that needs a configuration option IMHO.


Quote from: DrSuperGood on January 25, 2016, 04:19:13 AM
The ordering was changed to make powernet reconstruction easier. The substations and distribution transformers store their current tick power when saved. This is
As long as the change was fully assessed... But then taking care to update the existing comments would be appreciated.


Quote from: DrSuperGood on January 25, 2016, 04:19:13 AM
This logic was prone to overflows when a consumer used more than ~2 Gigawatts of power. One can argue if a consumer should be using so much power but if they did (pak128 steel mill with JIT2 on) then it broke. Same problem occurred with payment logic, which the new metering logic should have fixed.
So the animation fix was simply fixing the overflow? I thought you'd mentioned there was some other issue with the graphics getting stuck.

DrSuperGood

Quote
Having the limits explicitly mention in the code would be good as they were before. Far too often someone changes something and overflows a fixed point calc.
There were no limits mentioned before, that was probably why the overflow occurred. In any case I will look to define the limits more clearly. Do note that how the numbers are used may impose other limits, but that is not the problem of the powernet_t interface.

Quote
Fixed point doesn't magically allow a value between 0 and 1 for an integer. I'm sure you mean between 0 and the scaling factor - so say that.
It might not physically allow for such a value, but it logically does. People need to stop viewing fixed point as large numbers with scaling factor, and more as the actual numeric value they represent. The scaling constant already mentions the scaling applied, and is wired in for the scaling.

At some stage there really should be a fixed point type. Apparently you can use templates to create ones which optimize extremely well. Until then integers will are used with some manual scaling factor.

Quote
Minimize the already borderline excessive number of parameters in simuconf.tab... It just doesn't seem like something that needs a configuration option IMHO.
It need not be brought out, however the ability for it to be from a code perspective is still a good idea to have. I arbitrarily decided 10 seconds under the judgement it allows significant numbers to accumulate while still being fast enough people can quickly see the results. That does not make it the best payment period.

DrSuperGood

Another revision of the patch.

Fixed many of the above problems with comments.

The limits on powernet and the transformers is now more safe. They should be able to accept pretty much any values you can throw at them without breaking. One exception might be the reported power text, however that would be a UI issue only at pretty insane value cases due to how printf operates.

I rewrote the sync_step method of distribution transformers. Hopefully it should be more clear as to what is happening.

DrSuperGood

New revision of the patch. Updates the patch to be compatible with recent commits. Also further revises the sync step logic for distribution transformers to have fewer nested conditional statements and depend more on constant values.

Would be nice to know this has not been forgotten.

TurfIt

Not forgotten. I've been too busy the last couple weeks to look at Simutrans stuff.

DrSuperGood

Any progress towards this being approved?

TurfIt

Nope. IIRC there was something with the change to the step sequencing still bugging me, but haven't had a chance to setup a test. Too many irrelevant who cares excuses.... barring not throwing my computer out the window, I think I've a chance in the next couple weeks, but definitely starting from scratch since it's been so long since this was thought about.

TurfIt

Seems I only have r2, and the attachments are still AWOL. Please repost r4...

DrSuperGood

I posted this in January and.... Urg.

Note that it is highly unlikely to be compatible with current nightly due to how much time has passed since I made it. Specifically the save/load versions will cause problems and need updating.

prissi

Well, with your submitting power, maybe that fix for your patch is best done yourself?

DrSuperGood

Quote
Well, with your submitting power, maybe that fix for your patch is best done yourself?
It is on my todo list. However TurfIt seems very intent on reviewing it before I commit, possibly for the better just in case I missed something. The patch should work as long as a non-nightly save is loaded as pretty much just the save versions need updating.

Once he gives it the all clear I will update it and commit.

Further down the road I plan to look into JIT2 and possibly do a much larger revision of the system. Play testing showed some flaws that I hope can be corrected.

TurfIt

Ok, since you want to commit r5 today, here's where I got:

Looking at effect on JIT1 only. JIT2 logic is yours; I still think adding a feedback loop is a mistake...
The step sequencing bugging me appears to have been screwed up before this patch. Perhaps with r7662, perhaps before, but the comment about the factory getting the demand after a 2 tick delay added in 7662 is correct, but shouldn't be there as it shouldn't be working like that... I expect refixing will straighten out the mess of last_, this_, and next_ I previously mentioned. Perhaps commit this omitting the new save/loading logic to avoid a short lived save format?

Random thoughts from reading the patch:
Does the payment timer really need saving? Just make the payment before saving?
Payment timer better as per senke, and initialized random to prevent all senkes from paying at the same time? Synchronized payment spam.
Why a second iteration of senkes for the payment? iterating simutrans slists is horribly slow...
senke animation timers would be better as a static... they end up synced anyway after a reload.
powernet max limit enforcement moved from add_/sub_ to step_.  sync_step interrupting step could result in display of bad values. No current INTCHECKS between fab stepping and powerlines - future trap set.
pumpe and senke - set_net, rdwr, finish_rd set virtual - why?
leiting2 info - can't print uint64 with %u - why removing explicit casts where they're needed, and adding them where not??
1 << precision. WTF?   FFFF is a max, not 10000. Great way to get off by ones... and waste bits.
senke says 100% supplied when demand is 0. ?
pumpe, leitung says 100% usages when generation is 0. Pre patch display more sensible with that special cased.
POWER_TO_MW - whats wrong with #define? defined once, in one file. done.  now extern everywhere...

DrSuperGood

Quote
Looking at effect on JIT1 only. JIT2 logic is yours; I still think adding a feedback loop is a mistake...
I do plan to reduce the amount of feedback to be more logical rather than control based.

Quote
but the comment about the factory getting the demand after a 2 tick delay added in 7662 is correct, but shouldn't be there as it shouldn't be working like that...
It should not, but it is how it does, and has done for a long time.

Quote
I expect refixing will straighten out the mess of last_, this_, and next_ I previously mentioned. Perhaps commit this omitting the new save/loading logic to avoid a short lived save format?
So you want me to look into a way to reduce it to only 1 tick delay?

Quote
Does the payment timer really need saving? Just make the payment before saving?
Saving should not alter game state, only capture it. Like wise saving then loading should produce the same results as continuing to play does. I do understand Simutrans Save/load cycle is quite messy for reduced save complexity however there is no need to make it worse.

Quote
Payment timer better as per senke, and initialized random to prevent all senkes from paying at the same time? Synchronized payment spam.
So incrementing timers per senke per tick is better than 1 globally incremented timer? Also they were never timed or spaced randomly. Best would be to have the payment perfectly staggered to minimize payment performance impact however that can be done in the future.

Quote
Why a second iteration of senkes for the payment? iterating simutrans slists is horribly slow...
Did not consider caching at the time. Will change it to a single loop with a per element test.

Quote
senke animation timers would be better as a static... they end up synced anyway after a reload.
How would one update such a timer? I think that is why it has not been done already.

Quote
powernet max limit enforcement moved from add_/sub_ to step_.  sync_step interrupting step could result in display of bad values. No current INTCHECKS between fab stepping and powerlines - future trap set.
How can sync step interrupt step?

The tests are technically not necessary as power nets can cope with 2^32 different maximum power sources and producers before overflow occurs. If it does not overflow, which it almost certainly will not, then it gets clamped to the maximum value before the calculations are performed so no overflow can occur there as well. Both senke and pumpe do not care about the amount of power the factories tell them about since the power gets processed by the power net, which is designed to cope with the full numeric range, or in a way that cannot overflow.

Quote
pumpe and senke - set_net, rdwr, finish_rd set virtual - why?
Virtual set_net is needed to transfer the demand/supply between networks so it does not get discarded.

rdwr and finish_rd are already virtual due to inheritance. The change there is nothing more than eye candy to improve clarity. One does not need to use the virtual keyword for a method defined in a parent class as virtual but doing so makes it completely clear to programmers that the method is virtual as opposed to non virtual.

Quote
leiting2 info - can't print uint64 with %u - why removing explicit casts where they're needed, and adding them where not??
Not sure but that might have been as a result of me merging it last week, or an oversight. Will fix.

Quote
1 << precision. WTF?   FFFF is a max, not 10000. Great way to get off by ones... and waste bits.
What are you referring to? It sounds like part of the fixed point logic but maybe I am looking at the wrong part of the code.

Quote
senke says 100% supplied when demand is 0. ?
Any amount of power can fully satisfy 0 power demand. It is one way of handling the division by 0 case. 100% is as arbitrary as 0% and I personally think it makes more sense.

Quote
POWER_TO_MW - whats wrong with #define? defined once, in one file. done.  now extern everywhere...
There are a lot of things wrong with define... No type safety, no reference support, worse debug support, more prone to unpredictable results, etc. It is my understanding the reason define was used for constants was because it existed before the const keyword. In almost all cases now one should be using const where possible. Even macro functions intended for explicit in-lining can often be replaced with normal functions now as they will likely inline the same without making debugging a pain or being prone to strange compile errors.

I will be holding off committing for a day or so due to it needing to merged in again due to recent changes as well as to fix some of the above.

TurfIt

Quote from: DrSuperGood on January 09, 2017, 06:40:05 AM
So you want me to look into a way to reduce it to only 1 tick delay?
It should be done. It's not the result of this patch, but I think the fix will affect the saving which is added in this patch. So ideally it would be fixed to avoid a short lived saveformat, and the extra clutter to try and convert the saved values to whatever values are needed with the fix.
I found some debug output from 2010 when I rewrote the powerline logic in the first place. The stepping was working correctly then... not sure what/when broke.


Quote from: DrSuperGood on January 09, 2017, 06:40:05 AM
So incrementing timers per senke per tick is better than 1 globally incremented timer? Also they were never timed or spaced randomly. Best would be to have the payment perfectly staggered to minimize payment performance impact however that can be done in the future.
How would one update such a timer? I think that is why it has not been done already.
I think updating one payment timer per senke per step is fine. I don't think perfectly staggered is necessary - just init the timer to 0 when it's build. For loaded games without the value saved - init with random number.

There's currently 2 timers per senke per sync_step running for the animation, those can be moved to a single static one with the animations updated in sync - which is what happens anyways after a game load.  They'd be updated in the main karte sync_step, much like the patches current payment timer is updated in the main karte step.


Quote from: DrSuperGood on January 09, 2017, 06:40:05 AM
How can sync step interrupt step?
via INTCHECK.  There's not currently any in the fab/powerline stepping, but someone could add - hence why I said adding a future trap.


Quote from: DrSuperGood on January 09, 2017, 06:40:05 AM
The tests are technically not necessary as power nets can cope with 2^32 different maximum power sources and producers before overflow occurs. If it does not overflow, which it almost certainly will not, then it gets clamped to the maximum value before the calculations are performed so no overflow can occur there as well. Both senke and pumpe do not care about the amount of power the factories tell them about since the power gets processed by the power net, which is designed to cope with the full numeric range, or in a way that cannot overflow.
The display can only copy with powernets at max of 2^32<<POWER_TO_MW since the output values are 32 bit to avoid the inability to portably printf 64bit numbers.
The display is running in sync_steps, so could show before the step clamps it. Previously it was clamped upon adding, so never would be beyond max.


Quote from: DrSuperGood on January 09, 2017, 06:40:05 AM
One does not need to use the virtual keyword for a method defined in a parent class as virtual but doing so makes it completely clear to programmers that the method is virtual as opposed to non virtual.
All the existing code omits the virtual in that case. Would be nice to stick to one style...


Quote from: DrSuperGood on January 09, 2017, 06:40:05 AM
What are you referring to? It sounds like part of the fixed point logic but maybe I am looking at the wrong part of the code.
I presume it's the fixed point logic:

+ demand_factor = 1 << FACTOR_PRECISION;
+ supply_factor = 1 << FACTOR_PRECISION;

That ends up with demand_factor = 65536 as its '1' value. 65535 makes much much more sense to this binary mind.


Quote from: DrSuperGood on January 09, 2017, 06:40:05 AM
Any amount of power can fully satisfy 0 power demand. It is one way of handling the division by 0 case. 100% is as arbitrary as 0% and I personally think it makes more sense.
Or one can think if there's no demand, there's nothing to satisfy. IMHO, your way is more mathematically oriented, my way is more people friendly.

DrSuperGood

#33
Quote
It should be done. It's not the result of this patch, but I think the fix will affect the saving which is added in this patch. So ideally it would be fixed to avoid a short lived saveformat, and the extra clutter to try and convert the saved values to whatever values are needed with the fix.
I found some debug output from 2010 when I rewrote the powerline logic in the first place. The stepping was working correctly then... not sure what/when broke.
I will look into reducing it to 1 tick delay. Probably will not change much to do with save/load though.

Quote
I think updating one payment timer per senke per step is fine. I don't think perfectly staggered is necessary - just init the timer to 0 when it's build. For loaded games without the value saved - init with random number.
Needs another saved/loaded value though. Currently only 1 value is saved/loaded.

Quote
There's currently 2 timers per senke per sync_step running for the animation, those can be moved to a single static one with the animations updated in sync - which is what happens anyways after a game load.  They'd be updated in the main karte sync_step, much like the patches current payment timer is updated in the main karte step.
Will look into it. Would certainly improve efficiency.

Quote
via INTCHECK.  There's not currently any in the fab/powerline stepping, but someone could add - hence why I said adding a future trap.
This really needs better documentation...

Quote
The display can only copy with powernets at max of 2^32<<POWER_TO_MW since the output values are 32 bit to avoid the inability to portably printf 64bit numbers.
The display is running in sync_steps, so could show before the step clamps it. Previously it was clamped upon adding, so never would be beyond max.
Not possible to clamp before due to the need to correctly transfer power between nets during merger or splitting. If it was clamped the totals would be wrong.

Quote
That ends up with demand_factor = 65536 as its '1' value. 65535 makes much much more sense to this binary mind.
Except 65535 represents 0.9999847412109375... Fixed Point. It uses powers of 2 because simple shifts can be used instead of more complicated multiplication/division. "X << FACTOR_PRECISION" represents the value X in fixed point with FACTOR_PRECISION fractional bits. A value of 1 is used to represent 100% or full utilization.

Quote
Or one can think if there's no demand, there's nothing to satisfy. IMHO, your way is more mathematically oriented, my way is more people friendly.
This is needed so that factories get the full power bonus when on a network with no demand, like how mail and passengers also work when the factory is not producing anything. It makes sense from that point of view.

EDIT:
Making power nets only 1 tick delay requires large changes or a hacky work around...

The hacky work around would be to have another step all method run after stepping all power network objects which sends the power back to the factory. This is obviously bad for cache consistency and programming style. Logically this means that no values need to be retained in the power net for operation to work as the factories would store all the power values.

The large change approach would be to alter the relationship between factory and transformer so that factories push/pull power values rather than transformers. This changes the current flow path from {fab -> transformer -> network -> transformer (next tick) -> fab} to {fab -> network -> transformer -> fab}. The factory pushes its power demand/supply to the transformer which then pushes it through to the network. The power networks tick and compute the results. The transformers then tick only for power metering logic. Finally the factory ticks again and can pull the results of its last power demand/supply from the transformer which pulls them from the power network. Power demand/supply no longer have to be saved in the factory but still have to be saved at the transformers.

One problem with the large change approach might be cache consistency since factories need to dereference a transformer which then dereferences a power network only to have that transformer iterated later with a separate tick. Will this make much of a difference? Should I try this?

prissi

#34
Personally I think a payout before saving would be finem as there are lots of routines which have a different state after loading. It is highly unlikely (to put this mildly) thatthis get ever fixed without a complete rewrite of everything. And everything not save cannot go wrong when loading ... But we do not have a prepare_saving routine yet. (It may make sense though to add it to thing).

About virtual: Adding virtual when not needed may have undesired effects when it is removed in the main class for whatever reasoan. So I would only add virtual when really needed. Again, just my opinion.

AND: making teh slist a vector is rather trivial. Even more that unlike vehicles, senke and producer are not added taht often, so the main cost is indeed the iteration. But the templates makes taht change almost trivial.