News:

SimuTranslator
Make Simutrans speak your language.

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

Quote
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).
Is the file size reduction worth the reduction in save/load accuracy? The fractional accumulator would still need to be saved to prevent loss of fractional currency with each cycle although it would only have to be 32 bits instead of 64. As it is the removal of saving the net handle per power line object saves a huge amount of data compared with the addition of the new fields to save power and power accumulation. Maybe a better solution in the future would be a "small size" save setting which sacrifices save/load accuracy for reduced file size by cutting out things such as fractional accumulators and well defined orderings.

Quote
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.
Modifying the API by removing an in-use virtual method is going to have undesired effects in the first place. Changing API declarations that are in use is always a bad idea, which is why they are usually extended with old methods deprecated for a while before removal. My opinion is the added virtual makes it very clear that the method is a virtual method so may come from the parent and may be overwritten in children. Without the virtual declaration it might be a non virtual method or a virtual method inherited from the parent, with the only way to tell which being to either use a IDE which can track the inheritance of virtuals or to check all parent classes for a virtual declaration of the method.

At the time I had to manually track it down as I did not know how to use MSVC to look at all inherited virtuals. To be honest I still do not know how... This is probably a case of bad workman with good tools though.

Ters

Simutrans has an API? (Apart from the scripting, that is.)

I like adding virtual to all virtual functions to make it clear that they are virtual without having to look up the superclass (perhaps even the supersuperclass and so on, just to be sure). IDEs might make this a bit more trivial, but I have never found a C/C++ IDE I feel comfortable with, and I got the impression that I am not the only one around here that doesn't always use an IDE. Removing virtual from a method in the superclass which is overridden in a subclass without going through all the subclasses to check what these overrides are up to, is a bad idea whether these subclasses use virtual or not.

DrSuperGood

Quote
Simutrans has an API? (Apart from the scripting, that is.)
Maybe API was too strong a word. It is still an interface though and, as you said, modifying that interface without modifying (or at least checking) everything that depends on it is a bad idea.

The correct procedure is to extend or replace the interface and then deprecate the old parts, to be removed eventually. One should not be removing virtual methods from superclasses until one is sure nothing uses them anymore.

Ters

In Simutrans, there are few things that are difficult to change in one go, although I don't know how good C/C++ IDEs are when it comes to refactoring.

DrSuperGood

#39
I rewrote how powernets operate so that they are solved between factory ticks (no more 2 tick delay). As a result factories no longer save power (the transformers do). Hopefully power is easier to understand now as well as all that next, current and previous stuff is gone.

I plan to commit around Wednesday/Thursday this week (18/19th).

EDIT:
Note that power usage logging uses last tick's power satisfaction with the current ticks supply/demand which may result in slightly inaccurate numbers. However this is not really a mechanical problem but a minor UI problem and could be fixed by logging power usage before making a power demand (breaking out power logging from the general logging function). I will probably fix this before Wednesday.

kierongreen

Quote from: Ters on January 14, 2017, 08:26:00 AM
I like adding virtual to all virtual functions to make it clear that they are virtual without having to look up the superclass (perhaps even the supersuperclass and so on, just to be sure). IDEs might make this a bit more trivial, but I have never found a C/C++ IDE I feel comfortable with, and I got the impression that I am not the only one around here that doesn't always use an IDE. Removing virtual from a method in the superclass which is overridden in a subclass without going through all the subclasses to check what these overrides are up to, is a bad idea whether these subclasses use virtual or not.
Just to confirm I have also never used an IDE when working on Simutrans. Just a plain text editor and GNU compiler/linker.

DrSuperGood

I cannot connect to the SVN server (possibly IP banned again?) so I will hold off committing until Friday.

DrSuperGood

I have now committed the JIT2 and power patch. I ran a variety of stress tests using some of the server game maps and there was no obvious faults.

TurfIt

Completely broken for me. Straight out of the box - new map, JIT1, pak64, coal power plant, attached pumpe remains at 0 generation even when factory is producing.
Loading an old game - yoshi87 test game,   r8023 shows powernet with 10800 demand, 1242 supply. r8024, 16700 demand, 116 supply. Actually demand appears to be uninitialized - reloading the game gives different values including 4000000.


Also, format specifiers still wrong:

===> HOSTCXX obj/leitung2.cc
obj/leitung2.cc: In member function 'virtual void leitung_t::info(cbuffer_t&) const':
obj/leitung2.cc:352:37: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'uint64 {aka long long unsigned int}' [-Wformat=]
  buf.printf(translator::translate("Demand: %lu MW"), demand);
                                             ^
obj/leitung2.cc:354:41: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'uint64 {aka long long unsigned int}' [-Wformat=]
  buf.printf(translator::translate("Generation: %lu MW"), supply);
                                                 ^
obj/leitung2.cc: In member function 'virtual void pumpe_t::info(cbuffer_t&) const':
obj/leitung2.cc:610:74: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'uint64 {aka long long unsigned int}' [-Wformat=]
  buf.printf(translator::translate("Generation: %lu MW"), (uint64)(power_supply >> POWER_TO_MW) );
                                                 ^
obj/leitung2.cc: In member function 'virtual void senke_t::info(cbuffer_t&) const':
obj/leitung2.cc:869:69: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'uint64 {aka long long unsigned int}' [-Wformat=]
  buf.printf(translator::translate("Demand: %lu MW"), (uint64)(power_demand >> POWER_TO_MW));
                                             ^

Ters

Format specifiers for (u)int64 is a pain, since there is some differences between how GCC and MSVC treats them, which might be further complicated by mingw using a very old msvcrt.dll. You can use the PRIu64 macro.


printf("Demand: %" PRIu64 " MW");


Unfortunately, that doesn't work with translations.

TurfIt

Indeed. Which is why I explained why the previous limit was in place:
Quote from: TurfIt on January 13, 2017, 02:42:24 AM
The display can only cope 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.
but was ignored...

PRI seems to be for C99 only, and not any C++. MinGW header complains:

* MS runtime does not yet understand C9x standard "ll"
* length specifier. It appears to treat "ll" as "l".
* The non-standard I64 length specifier causes warning in GCC,
* but understood by MS runtime functions.

Easiest to just not print a 64bit number!

Ters

As far as I can tell, %lu is just plain wrong anyway, except when compiling for 64-bit Linux and possibly MacOS X. If we are to print 64-bit values in localizable strings, we need to come up with our own formatted string output solution. printf and friends are only mandated by the standard to know about the classic C types, whose sizes are implementation defined, with compile-time macros to support the C99 fixed width integer types. The streams in C++ doesn't do localized formatting as far as I know (I don't like their syntax, so I haven't used them much).

DrSuperGood

#47
Quote
Completely broken for me. Straight out of the box - new map, JIT1, pak64, coal power plant, attached pumpe remains at 0 generation even when factory is producing.
Loading an old game - yoshi87 test game,   r8023 shows powernet with 10800 demand, 1242 supply. r8024, 16700 demand, 116 supply. Actually demand appears to be uninitialized - reloading the game gives different values including 4000000.
I do initialize supply in both of the constructors. I messed up the legacy JIT1 logic and forgot to test... Will be fixed.

Demand is quite possibly broken when loading older saves as I forgot to initialize it in the load constructor. This oversight will be fixed and code reorganized to match the other constructor.

There was also an oversight with power net destruction. Deleting a transformer would not update the net correctly. Will also be fixed.

Quote
Also, format specifiers still wrong:
The incorrect net ID printing statement confused me. I thought it was printing 64 bits due to it printing a pointer when it is only printing the first 32 bits of the pointer (which is clearly wrong for 64 bit builds).

Quote
Format specifiers for (u)int64 is a pain, since there is some differences between how GCC and MSVC treats them, which might be further complicated by mingw using a very old msvcrt.dll. You can use the PRIu64 macro.
Quote
PRI seems to be for C99 only, and not any C++. MinGW header complains:
Problem is that "ll", which is the correct formatter I should have used as it is for types which can at least represent the range of a 64 bit integer, is only supported by C99. In C++, C99 (or at least that part of it) is only supported by C++11. Both modern GCC and MSVC do support C++11, at least sufficiently for that feature to work. Simutrans does not allow any C++11 features though...

Quote
Easiest to just not print a 64bit number!
Easier, certainly is. But correct? I do not think so.

In any case I will fix that and add clamping to the display. It is not a perfect solution but better than nothing until Simutrans advances to use C++11 or newer or some other way to support printing of 64 bit integers.

prissi

You can always cast the int64 to double and printf the double. That is the way simutrans saved uint64, and it works since the double has 80 bit precision and rounding errors must not occur (if implemented correctly ... )

DrSuperGood

Pushed a commit that should fix the issues. I went with double type in the end as it is the most flexible (less likely to need change in future) while still supporting the full required range, even if only approximately.

TurfIt

#50
Generators now work.
Loads broken --> factories still demanding power when output stores >= 75%.


Quote from: DrSuperGood on January 23, 2017, 10:10:45 PM
Easier, certainly is. But correct? I do not think so.
How's it not correct?
Reading 3891261344 is hard enough. 47892635782336527632 impossible.  That's why thousand separators were born 3 891 261 344. Or units  3891261344 MW > 3891261 GW > 3891 TW. At those magnitudes, who cares about the rest of the number. So I don't see why it's needed to print a full 64 bit number into a UI.


---
From nightly build farm:

error 23-Jan-2017 08:07:08 obj/leitung2.cc: In static member function 'static void senke_t::step_all(uint32)':
error 23-Jan-2017 08:07:08 obj/leitung2.cc:642:39: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

DrSuperGood

QuoteLoads broken --> factories still demanding power when output stores >= 75%.
The new power API requires being explicitly set into a low power mode. For JIT2 this was no problem but for JIT0/1 it was because it did not turn off ordering. In theory there could be an optimization here where power is only set if there is a change but that can come at a later time.

QuoteReading 3891261344 is hard enough. 47892635782336527632 impossible.  That's why thousand separators were born 3 891 261 344. Or units  3891261344 MW > 3891261 GW > 3891 TW. At those magnitudes, who cares about the rest of the number. So I don't see why it's needed to print a full 64 bit number into a UI.
It is not correct if it prints out the wrong amount of power because it has overflowed, eg displaying 0 power when it is some really huge number. One should really have an automatic rescaling of units, however that is a feature for another time.

QuoteFrom nightly build farm.
Its reporting the wrong line which is kind of strange. I guess some kind of inline pre-optimization is at work however it should really point at 636.

Ters

Quote from: DrSuperGood on January 23, 2017, 10:10:45 PM
Problem is that "ll", which is the correct formatter I should have used as it is for types which can at least represent the range of a 64 bit integer, is only supported by C99. In C++, C99 (or at least that part of it) is only supported by C++11. Both modern GCC and MSVC do support C++11, at least sufficiently for that feature to work. Simutrans does not allow any C++11 features though...

No, "ll" is the wrong formatter on 64-bit Linux. A single "l" is correct there, due to regular longs being 64-bit there. This is why the C99 standard has introduced the macros. Secondly, printf is part of the C runtime library, not the compiler. GCC relies on a system provided C runtime (which is the norm on *NIX), while MSVC has, up until very recently, used a compiler provided C runtime. The only C runtime "provided" by Windows up until now, is basically the MSVC6 C runtime. Hopefully, someday the mingw64 team will look into the new Universal CRT, which is an up-to-date C runtime provided (note, no quotes, this time it is for real) by Windows.

DrSuperGood

QuoteNo, "ll" is the wrong formatter on 64-bit Linux.
It is the right format type as it is standardized since C99 (part of C++11)... A "long long" uses the format type specifier "ll" and is capable of containing at least the numeric range of a int64_t and uint64_t with appropriate formatter sign. It will work correctly on both 32bit and 64bit Linux to print a 64 bit integer value as it is required by C99 to do so.

On the other hand "l" is specified to only support the long type (same minimum numeric range as int32_t and uint32_t). However nothing stops one from making a long support the required range of long long which is what you might be referring to on 64-bit Linux and how the l format type could possibly work. That said this is not portable as it is relying on supporting a numeric range outside the language requirements.

Ters

Quote from: DrSuperGood on January 24, 2017, 06:00:09 PM
It is the right format type as it is standardized since C99 (part of C++11)... A "long long" uses the format type specifier "ll" and is capable of containing at least the numeric range of a int64_t and uint64_t with appropriate formatter sign. It will work correctly on both 32bit and 64bit Linux to print a 64 bit integer value as it is required by C99 to do so.

I wasn't aware that long long was required to be at least 64-bit. In fact, I have never encountered rules on minimum sizes in terms of bits, only 1 == sizeof(char) <= sizeof(short) <= sizeof(int) <= sizeof(long) <= sizeof(long long), except I may not have seen one with long long before. That's what I get for learning C and C++ from discussion boards and a few random sites around 2000. However, by the same reasoning, "ll" seems also correct for int8_t. And what are these macros for, then?

It does however seem that someone working on mingw64 started looking into supporting the Universal CRT. Hopefully they will switch some day. Unfortunately, it looks like the nightly build server is likely using mingw32.

DrSuperGood

QuoteI wasn't aware that long long was required to be at least 64-bit.
It is defined by a minimum value range for long long and unsigned long long. However logic dictates, at least with our current understanding of computer science, that it has to be at least 64 bits to fulfil the value range requirement as an integer type.

QuoteAnd what are these macros for, then?
An alternative way of doing it. Just like in theory using a C99 compliant compiler one does not need to use uint64_t and such as a unsigned long long has a similar meaning. One might still want to as it is more readable, which was possibly the intention of the macros.

prissi

Long long could be any size. I had an DSP (TMS320 family) where int was 40 bit and long 40 bit and long long also 40 bit. That was why portab.h and later cstdint.h invented. So this DSP compiler would have had no int64_t, only int_least64_t. (Since this compiler was more K&R with few extensions (like understanding long long), this header was not there either.)

The PDP10 had a native byte size of 9 bit; and you can still buy processors with 12 bit native chars (and hence 24/48 bit word/long). Some more examples here http://stackoverflow.com/questions/2098149/what-platforms-have-something-other-than-8-bit-char It misses many DSP, which are very often native 10 or 12 bit or even 24 bit (depending on the AD/DA concerters) And those are programmed almost exclusively by C and to a much lesser degree by C++.

DrSuperGood

QuoteLong long could be any size. I had an DSP (TMS320 family) where int was 40 bit and long 40 bit and long long also 40 bit. That was why portab.h and later cstdint.h invented. So this DSP compiler would have had no int64_t, only int_least64_t. (Since this compiler was more K&R with few extensions (like understanding long long), this header was not there either.)
Only in non C99 compilers. Any compiler that is C99 compliant, as defined by the C99 standard, must have a long long type of at least 64 bits to support the minimum required value range. However it could be 80 bits or any other value as long as it is not less than 64 bits.

QuoteThe PDP10 had a native byte size of 9 bit; and you can still buy processors with 12 bit native chars (and hence 24/48 bit word/long). Some more examples here http://stackoverflow.com/questions/2098149/what-platforms-have-something-other-than-8-bit-char It misses many DSP, which are very often native 10 or 12 bit or even 24 bit (depending on the AD/DA concerters) And those are programmed almost exclusively by C and to a much lesser degree by C++.
I do not think one really cares that much about portability with respect to systems that have non 8 bit chars, except maybe between systems with the same char size. How does one read a file portably in such systems in the first place? Dealing with different endians is bad enough but how do 8 bit char files translate to non 8 bit char files? I do not even think the conversion is defined or necessarily meaningful.

prissi

The DSP had no real file IO, it had only fgetc and fputc in its library, and string reading were performed by calling fgetc repeatatively. Reading binary data from other machines was obviously not so meaningful.

Anyway, this thread goes way off course now ...