Hi everybody

[socram8888]

Can I set Checkstyle's lineSeparator to "LF"? Currently it is set to "system" and it causes loads of warnings when grabbing the source code using the Git from Cygwin, which uses "LF" for newlines, while the default system newline (and therefore the one used by Checkstyle) is "CRLF".

Also, is there any convention on formatting for configuration files? Checkstyle configuration file uses a mix of tabs and spaces and indentation in blank lines.

 

[Immortius]

Uh, no. Correctly configured, Git changes line endings to be correct for your system on check out by default, so for someone working under Windows they will be CRLF. Someone else under Linux will have LF. So system is the correct option for CheckStyle.

You will need to look at your Git configuration and ensure it is correct for your setup. See http://adaptivepatchwork.com/2012/03/01/mind-the-end-of-your-line/

On the formatting of config files - my preference is for all spaces, unless the consumer of the file requires tabs.

Edit: looks like a new system has been added where you can put the settings within the repository itself, so we should look into that.

 

[socram8888]

https://github.com/socram8888/Terasology/tree/fastpow Here's the branch I've added the new power function. I've also fixed the formatting of the Checkstyle configuration file (replacing tabs with spaces and removing these from blank lines), and moved Matrix utils to their own class (was in a TODO comment, and I did it before adding the power function to simplify TeraMath)

 

[Cervator]

Cool - should we merge that already then? Immortius ?

I got started late tonight so I won't cut over the main branch until tomorrow (hopefully!), leaving a bit of time.

 

[Immortius]

Looks good to me. Would still like some unit tests, but isn't crucial.

 

[socram8888]

Looks good to me. Would still like some unit tests, but isn't crucial.

That's the reason I've hadn't issued a pull request yet, I want to add those tests first

I've modified a bit the FastRandom function (randomCharacterString is now truly random and does not follow any kind of pattern, and uses a local array rather than a StringBuilder, as we know beforehand how many characters do we need), and added a fix for FastRandom objects with seed "0" always returning "0".

The fix for the later was indeed quite simple: https://github.com/socram8888/Terasology/commit/6e702603810263898bb54788ba34dc2ee7a0f3f0

EDIT: I've been thinking and I can't really think a good test for the power function. I mean, it's just a bunch of multiplications!

 

[Cervator]

Hehe, yes, that was a pretty simple fix

Also, throw a PR together so I can grab it tonight before doing the restructure branch. If you come up with some unit tests to add just put them in the same branch and the PR will update automatically

As for tests, yeah, they're pretty elementary, but I guess you can still assert pow(2,2) == 4 or something

 

[Skaldarnar]

Think of the edge cases Immortius already proposed

specifically around the edge scenarios like exp == 0, -1, 1, 2, and base == -1, 0, 1, 2 (for some of those cases you might need to hand over to Math.pow)

 

[Immortius]

That's the reason I've hadn't issued a pull request yet, I want to add those tests first

I've modified a bit the FastRandom function (randomCharacterString is now truly random and does not follow any kind of pattern, and uses a local array rather than a StringBuilder, as we know beforehand how many characters do we need), and added a fix for FastRandom objects with seed "0" always returning "0".

Please be careful not to over-optimise. If something isn't performance critical it is better off being maintainable than slightly faster. Is the random string generator used much? I think it is only used to generate a world seed.

EDIT: I've been thinking and I can't really think a good test for the power function. I mean, it's just a bunch of multiplications!

The key to unit tests is you are testing behavior, not implementation. The pow method should have the same behaviour regardless of whether the implementation uses multiplication or emails a renowned mathematician.

Ideally you would test all possible inputs, but that is impractical. So instead you need to test a representative subset, with a focus on border values, such as 0 or Max Int. This is where knowledge of the implementation can help inform what values are border values.

One question that can help to consider is whether a change that breaks the contract of the method will cause a unit test to fail. If I, in a misguided attempt to improve performance, remove the if block for negative exp, or remove the incrementing of base, or change the method to always return 1, the unit tests should detect this. Or essentially, the existence of every line and behavior of the method should be supported by a test. Contrary wise, if I replace the implementation with a completely different one that still passes the tests I should be confident that it does everything a pow function needs to do.

 

[socram8888]

I've found some weird stuff in org/terasology/utilies/procedural/EPNoise.java. For some reason it tries to do modulus with doubles:

for (int i = 0; i < 256; i++) {
            if (random) {
                noiseTable[i] = (int) (rand.randomDouble() % 256); // Shouldn't this be rand.randomAbsInt(256)?
            } else {
                noiseTable[i] = i;
            }
        }

And it is full of code like this, though I don't understand what is its purpouse. Was FastRandom randomDouble modified at some point and this was left unchanged?

 

[Cervator]

Nice catch. Doesn't look like it was changed recently, the file's history doesn't show anything relevant for the last 6 months (before which the file was somewhere else - might be trickier to find that history)

I'm thinking it is part of the Perlin variant introduced by Esereja quite a while ago. With no intent to offend it may just be messy code - that was sort of typical in those contributions ...

Unless we can get a response from Esereja I'd go ahead and optimize soon if you're up for it. Although I'm not really sure if that variant has a solid future - I could see us go with more of a customizable Perlin setup a la the version MPratt supplied, where variation is provided more through parameters than custom Perlin code (similar situation to the on and off-again Hills PR). That would be part of the big world gen overhaul, but that whole thing is on hold pending contributor availability

 

[socram8888]

Actually I already started fixing the class just after posting that message

I've already rewritted most of the class, removing magic values with proper constants, but for some reason I can't compile the "multiplayer" branch of Git. It seems that there are some missing classes that are used by the Fence mod.

EDIT: Nevermind, I just had a broken clone. Redownloading it did the job.

 

[socram8888]

https://github.com/socram8888/Terasology/commit/fb86d65f0c0756539e31348c2a4d4aa851158103

I've created a new branch to rename methods from FastRandom to more meaningful names, for instance:
randomInt() -> signedInt()
randomIntAbs() -> unsignedInt()
randomFloat() -> signedFloat()
randomPosFloat() -> unsignedFloat()
randomCharacterString(int len) -> characterString(int len)

For a newcomer, this:

double offset = random.unsignedDouble() * 3.0;

is much more readable than:

double offset = 1.5 * (random.randomDouble + 1.0);

Which would think: "why on earth are you adding one? wouldn't that return a biased result?". I did when I rewrote the L-system tree generator. Even Immortius has got confused with them: https://github.com/MovingBlocks/Terasology/commit/102b5dfbb0db956ff66e45c31fc1e05ba5013d66

 

[socram8888]

Rewrital finished: https://github.com/socram8888/Terasology/commit/791a82704c2d2d88e9eb5c7a42db5703a5cf9010

 

[Immortius]

https://github.com/socram8888/Terasology/commit/fb86d65f0c0756539e31348c2a4d4aa851158103

I've created a new branch to rename methods from FastRandom to more meaningful names, for instance:
randomInt() -> signedInt()
randomIntAbs() -> unsignedInt()
randomFloat() -> signedFloat()
randomPosFloat() -> unsignedFloat()
randomCharacterString(int len) -> characterString(int len)

For a newcomer, this:

double offset = random.unsignedDouble() * 3.0;

is much more readable than:

double offset = 1.5 * (random.randomDouble + 1.0);

I am unsatisfied with this, sorry. It still isn't consistent with other existing Random number generators, such as Java's Random or .Net's Random. Additionally I think unsigned and signed are still potentially confusing - they all return signed values just some are restricted to positives. Three there is no need for an "unsignedInt" variation anyway, the only place it is used is an error.

What I would suggest is

nextInt() // Int from entire range
nextInt(int range) // Int from 0 inclusive to range exclusive
nextInt(int min, int range) //Int from min inclusive to min + range exclusive
nextIntMinMax(int min, int max) //Int from min inclusive to max inclusive
 
nextDouble() // Double from 0 inclusive to 1 exclusive
nextDouble(double range) // Double from, 0 inclusive to range exclusive
nextDouble(double min, double range) // Double from min inclusive to min + range exclusive
 
// And same again for float and so forth

This brings it in line with Java's Random (which it should implement).

I think it is clearer still to have

double offset = random.nextDouble(1.0, 5.0);

than

double offset = 5.0 * random.signedDouble() + 1.0;

And of course you can do

double offset = random.nextDouble(-5.0, 10.0);

Which would think: "why on earth are you adding one? wouldn't that return a biased result?". I did when I rewrote the L-system tree generator. Even Immortius has got confused with them: https://github.com/MovingBlocks/Terasology/commit/102b5dfbb0db956ff66e45c31fc1e05ba5013d66

The other probelm with FastRandom is it uses modulo to handle int ranges, which does introduce bias. To give a simple example, lets say we were doing nextInt(100) based on a random number generator with a byte for a seed. Doing

return Math.abs(nextByte()) % 100;

has two problems.

1. The range of a byte is -126 to 127. When getting the absolute of the byte, 127 and 0 are both half as likely to occur as the other numbers.
2. By doing modulo 100, 0-27 get over represented. In fact, 1-26 are 2x more likely than than 28-99.

 

[socram8888]

Inhereting Random isn't a good idea. Java Random is synchronized, and that synchronization has an overhead, which might be negligible for some applications, but not for a class called FastRandom that is used all over the code. Check this article talking about the multiply-with-carry RNG (also made by George Marsaglia, like the Xorshift currently used) implemented in Java, which talks about how much does the synchronization hurts performance: http://www.javaprogrammingforums.com/blogs/helloworld922/11-complimentary-multiply-carry-better-way-generate-pseudo-random-numbers.html. It would be a better idea to either just rename the methods, without extending Random, or create an abstract class or interface, without synchronization, for RNGs, and have several kind of them (Xorshift, CMWC, a wrapper around Java's...)

I noticed the biasing too (see https://github.com/socram8888/Terasology/commit/791a82704c2d2d88e9eb5c7a42db5703a5cf9010#diff-32ba3629db1ce44264dd0ba3c40449a3R79), but as I already pointed out, I guess in a class with a name like this, speed is more important than a slighty biasing.

 

[Cervator]

I don't see a reference to inheriting Random? Just a suggestion to make stuff look like Random some more, which sounds good

Also: Cool stuff, keep it up!

 

[socram8888]

I might have misunderstood this:

This brings it in line with Java's Random (which it should implement).

 

[Immortius]

Sorry, I thought Random was an interface not a class. I guess it is from the pre-sane-Java era.

The biasing can be fixed by basing all the range restricted methods off of nextDouble(), which returns between 0 and 1 (assuming there is no bias there). So nextInt(int range) would be

{
    return (int) (range * nextDouble());
}

There's no point to being fast if you are not first correct. Otherwise:

public int nextInt() {
    return 4; // Chosen by die roll, guaranteed random
}

Interesting side-note: nothing about Random's API guarantees that it is thread safe, although the implementation in Oracle's JVM is. So technically you should synchronize it manually, as someone could be using a JVM where it isn't thread safe.

 

[Cervator]

I might have misunderstood this:

That's what I get for not sleeping enough, hehe