Print

Please login or register.
1 Hour 1 Day 1 Week 1 Month Forever
Login with username, password and session length

[Burning Skull]

That code is clearly incomplete and dangerous. That is more secure version:

Code: [Select]

private int getVPFromSpecialCards()
{
    int num = 0;
    if (this.numFairgrounds > 0)
    {
        if (this.numOfDifferentCards > 0)
        {
            num += (this.numFairgrounds * 2) * (this.numOfDifferentCards / 5);
        }
    }
    if (this.numSilkroads > 0)
    {
        if (this.numVictoryCards > 0)
        {
            num += this.numSilkroads * (this.numVictoryCards / 4);
        }
    }
    if (this.numVineyards > 0)
    {
        if (this.numActionCards > 0)
        {
            num += this.numVineyards * (this.numActionCards / 3);
        }
    }
    if (this.numDukes > 0)
    {
        if (this.numDuchiesCards > 0)
        {
            num += this.numDukes * this.numDuchiesCards;
        }       
    }
    if (this.numGardens > 0)
    {
        if (this.numCards > 0)
        {
            num += this.numGardens * (this.numCards / 10);
        }
    }
    if (this.numFeodums > 0)
    {
        if (this.numSilvers > 0)
        {
            num += this.numFeodums * (this.numSilvers / 3);
        }
    }
    return num;   
}


[michaeljb]

There's this ridiculous function that calculates a factor that affects how long cards stay up in the air:

Code: [Select]

public static float getFlightTimeMultiplier(string source, string destination)
{
    float num = 1f;
    if ((source != null) && (destination != null))
    {
        if (source == destination)
        {
            if (source == "play")
            {
                return 0.6f;
            }
            return (!source.Contains(".0") ? 0.1f : 0.4f);
        }
        if (source.StartsWith("deck"))
        {
            if (destination.StartsWith("hand"))
            {
                num = !(destination == "hand.0") ? 0.3f : 0.9f;
            }
            return num;
        }
        if (((destination == "globalReveal") || (destination == "globalRevealTwo")) || ((destination == "globalRevealBlackMarket") || destination.StartsWith("reveal")))
        {
            return 0.45f;
        }
        if (source.StartsWith("discard"))
        {
            if (destination.StartsWith("deck"))
            {
                num = !(destination == "deck.0") ? 0.3f : 0.5f;
            }
            return num;
        }
        if (source.StartsWith("hand"))
        {
            if (destination.StartsWith("discard"))
            {
                num = !(destination == "discard.0") ? 0.4f : 0.8f;
            }
            return num;
        }
        if (destination == "play")
        {
            return (!(source == "hand.0") ? 1f : 0.5f);
        }
        if ((source == "play") && destination.StartsWith("discard"))
        {
            num = !(destination == "discard.0") ? 0.5f : 0.7f;
        }
    }
    return num;
}

So I don't know much about RE, and I've only done a little work in school with C#, but some of this *has* to be the result of you only being able to look at the code after it was compiled and then decompiled, right? Like, the compiler could make optimizations that are less readable, right? For my own sanity I just need to know a human didn't actually write that code.

But I'm afraid it doesn't make sense for the compiler to use nested ifs sometimes and ternaries other times...and why on earth is the ternary always written with "!" in front of the condition it's checking? Surely a compiler optimization would not have that, and surely a human would not write it that way, because code should be written to be readable.

 

edit: So I've gone through and removed nested ifs from the code; it's still not great, but at least I can read it. I'm not actually compiling/running/testing this so I'm not 100% sure that it's correct, but it should be pretty close.

Code: [Select]

public static float getFlightTimeMultiplier(string source, string destination)
{
    if (source == null || destination == null)
    {
        return 1f;
    }

    if (source == destination && source == "play")
    {
        return 0.6f;
    }

    if (source == destination && source.Contains(".0")
    {
        return 0.4f;
    }

    if (source == destination)
        return 0.1f;
    }

    if (source.StartsWith("deck") && destination == "hand.0")
    {
        return 0.9f;
    }

    if (source.StartsWith("deck") && destination.StartsWith("hand"))
    {
        return 0.3f;
    }

    if (source.StartsWith("deck"))
    {
        return 1f;
    }

    if ((destination == "globalReveal") || (destination == "globalRevealTwo") || (destination == "globalRevealBlackMarket") || destination.StartsWith("reveal"))
    {
        return 0.45f;
    }

    if (source.StartsWith("discard") && destination == "deck.0")
    {
        return 0.5f;
    }

    if (source.StartsWith("discard") && destination.StartsWith("deck"))
    {
        return 0.3f;
    }

    if (source.StartsWith("discard"))
    {
        return 1f;
    }

    if (source.StartsWith("hand") && destination == "discard.0")
    {
        return 0.8f;
    }

    if (source.StartsWith("hand") && destination.StartsWith("discard"))
    {
        return 0.4f;
    }

    if (source.StartsWith("hand"))
    {
        return 1f;
    }

    if (destination == "play")
    {
        return 1f;
    }

    if (source == "play" && destination == "discard.0")
    {
        return 0.7f;
    }

    if (source == "play" && destination.StartsWith("discard"))
    {
        return 0.5f;
    }

    return 1f;
}

It's not just ugly and slow. Man. Let's say I have a typo in one of the strings. With the enumerated types, it doesn't compile. Oh, typo, fixed, compile. With these strings, it compiles, it runs, you've got a bug. In this case the bug is that the time it takes a card to go from one place to another isn't the intended time in certain situations, so, it does not loom large, unless it's awful enough to notice. But you know. If they're doing this here they're doing it in other places.

Say what you mean! Is my first rule of computer programming. When you don't mean a string, don't use a string.

What do you know, I think there's a bug with this function when source is "hand.0" and destination is "play". The code posted by SCSN contains this snippet:

Code: [Select]

if (destination == "play")
{
    return (!(source == "hand.0") ? 1f : 0.5f);
}
This snippet wants to return 0.5f if source is "hand.0" and destination is "play", but the chunk above that, that starts with

Code: [Select]

if (source.StartsWith("hand")) will return 1f.

[pedroluchini]

TL;DR: Don't judge a programmer by his decompiled code.

So I don't know much about RE, and I've only done a little work in school with C#, but some of this *has* to be the result of you only being able to look at the code after it was compiled and then decompiled, right? Like, the compiler could make optimizations that are less readable, right? For my own sanity I just need to know a human didn't actually write that code.

I can't speak for C#/CLR, but in the world of C/assembly that's exactly what happens. For instance, these two snippets of code...

Code: [Select]

if (a && b)
{
   do_stuff();
}
rest_of_program();

Code: [Select]

if (a)
{
   if (b)
   {
      do_stuff();
   }
}
rest_of_program();
...would be indistinguishable when compiled to machine code; i.e., both of them would become something like this (very roughly speaking):

Code: [Select]

compare a, 0;
jmp_if_eq after_if;
compare b, 0;
jmp_if_eq after_if;
call do_stuff;
after_if:
call rest_of_program;
A decompiler has to look at that sequence of instructions and try to figure out which logic structures would produce such a sequence, and the result is usually incorrect (i.e., it doesn't match the structure that the C programmer originally wrote).

I have absolutely zero sympathy for MF and their piece of junk software, but criticizing their craftsmanship based this reverse-engineered chunk of code is misguided. We have plenty of legitimate reasons to criticize their craftsmanship, after all.

[SCSN]

The thing is that if you look at the CIL code (which I can post if anyone is interested), the C# code generated by the decompiler is close to the upperbound of reasonableness of code that could have produced this particular CIL sequence, in the sense that the decompiler always settled for the former of these two:

Code: [Select]

if (a && b)
{
   do_stuff();
}
rest_of_program();

Code: [Select]

if (a)
{
   if (b)
   {
      do_stuff();
   }
}
rest_of_program();

In other words: there are a lot of ways in which their actual code could be more convoluted, but it can't be much simpler. The unnested code by michaeljb, for example, would have produced very different CIL code.

[Witherweaver]

About string comparisons, I actually had the same bug Donald was talking about.  It took me a while to figure out why I was getting weird results.  Though this program is a bit of a quick proof-of-concept hack, and that's probably one of the minor issues with how I wrote it.  It needs to be rewritten before actually being used.  But I guess that's why we have developers. 

[popsofctown]

Well, to be an apologist, suppose that "getnum of Duchy cards" and "getnum of different cards" are really slow operations.  In that case, checking if the special victory card is present at all will speed up execution, since that function won't get called at all.  So maybe it's worth making the code less readable.

The only issues with that are:
1. The function is only getting called about 400 times per game or so.
2. It's kinda crazy to envision a scenario where those operations are hard.
3. If those operations DO become hard, then an additional check whether the special victory card or Black Market is in the kingdom at all should come before the check for the card being in the deck, because that check itself would also be slow.

EDIT:Wait, did michaeljb find a bug where the code is always spending double animation time for cards in hand? Lololol...

[Witherweaver]

Why are they numDuchiesCards instead of numDuchies, like the others?  Duchy is name of card, not  a type.

[Schneau]

Gotta make sure you never multiply by zero:

Code: [Select]

private int getVPFromSpecialCards()
{
    int num = 0;
    if (this.numFairgrounds > 0)
    {
        num += (this.numFairgrounds * 2) * (this.numOfDifferentCards / 5);
    }
    if (this.numSilkroads > 0)
    {
        num += this.numSilkroads * (this.numVictoryCards / 4);
    }
    if (this.numVineyards > 0)
    {
        num += this.numVineyards * (this.numActionCards / 3);
    }
    if (this.numDukes > 0)
    {
        num += this.numDukes * this.numDuchiesCards;
    }
    if (this.numGardens > 0)
    {
        num += this.numGardens * (this.numCards / 10);
    }
    if (this.numFeodums > 0)
    {
        num += this.numFeodums * (this.numSilvers / 3);
    }
    return num;
}
At least they tried...

This code is awful and should have never been written. Using a variable called "num" for victory points? That's begging to cause problems.

[popsofctown]

Why are they numDuchiesCards instead of numDuchies, like the others?  Duchy is name of card, not  a type.

There are some private data members we don't know about, that increment the number of Duchies in the kingdom that your deck represents as you buy more cards.  For instance, if you buy both a Farming Village and a Market, that combination of economy and commerce expands the your numDuchies to 2 as the imaginary kingdom represented by your deck inevitably expands due to that robust development.  The Duchies are stored as a linked list of complex objects, each one has a name, the one generated by your Farming Village and Market might, for instance, be called "Nottingham".  It also has data about the people that live there, like Beatrice, who was born on a cold winter the same year her father was Swindled out of his Woodcutting business and had to take up the dirty act of Smuggling just to get by, and she works in the Farming Village now that she has grown older, and sells her goods for whatever she can get in the Market.  She has some pointers that correspond to men she is attracted to in your opponent's Duchies, but crossdeck romance is forbidden, so these pointers are never used.  It is a deep metaphor for the things we want but cannot have.  Eventually she marries the new Woodcutter from the same kingdom and they exchange some data.

Once the game is over, all this data is destroyed and there is no way to view any of it.  However, you are all benefiting from a rich backstory from the growing Duchies mechanic in ways you can't possibly understand, and it adds meaning and substance to your dominion games.

This is totally unrelated to the number of Duchy cards in your deck, which is totally different, those are worth VP, so they have to be named differently.  That's just good coding.


Now you guys know, the code is fine, you just don't have the big picture.

[michaeljb]

A decompiler has to look at that sequence of instructions and try to figure out which logic structures would produce such a sequence, and the result is usually incorrect (i.e., it doesn't match the structure that the C programmer originally wrote).

I thought that this would have to be the case; it's like playing telephone with code.

Now I'm just curious about why all the ternaries look like "not X? then A else B" instead of "X? then B else A". It seems like making it "not X" is just adding an extra operation. Maybe it is actually an optimization because the value X needs to be evaluated like a boolean (1 or 0) anyway, and "!" is a good way to convert it to boolean (1 or 0)?

Now I'm thinking about the multiply-by-zero example more, could that be the compiler's method of short-circuiting multiplication by 0? We can't see the types of those properties here, they could all be unsigned.

[Donald X.]

Well, to be an apologist, suppose that "getnum of Duchy cards" and "getnum of different cards" are really slow operations.  In that case, checking if the special victory card is present at all will speed up execution, since that function won't get called at all.  So maybe it's worth making the code less readable.

It's not a function here, it's a variable. If this isn't C and it's somehow a function, it's a function both times (much worse in your scenario), since it looks the same.

Code: [Select]

if (this.numFairgrounds > 0)
    {
        num += (this.numFairgrounds * 2) * (this.numOfDifferentCards / 5);
    }
Some other routine counted Fairgroundses, now this one looks at that number. Why another routine calculated it and stored it when it's only used here, I couldn't tell you. Maybe they expect a future expansion with counting copies of Fairgrounds as a theme.

[eHalcyon]

Some other routine counted Fairgroundses, now this one looks at that number. Why another routine calculated it and stored it when it's only used here, I couldn't tell you. Maybe they expect a future expansion with counting copies of Fairgrounds as a theme.

Maybe they keep running tallies of all those things all the time no matter what.

[SCSN]

Let's say I have a typo in one of the strings.

You mean like accidentally typing "possesion" rather than "possession"? These guys are pros, they totally saw that one coming:

Code: [Select]

public void setPlayerAvatarStates(string state)
{
    if (this.pState != state)
    {
        this.pState = state;
        if (this.pState == "turn")
        {
            this.pBgAvatar.color = Color.yellow;
        }
        else if (state == "normal")
        {
            this.pBgAvatar.color = Color.grey;
        }
        else if (state == "possesion")
        {
            this.pBgAvatar.color = Color.magenta;
        }
    }
}
Now there's just hoping that they don't suddenly employ someone who knows how to spell, or we'd risk missing out on those lovely magenta avatar backgrounds during "possesion" turns.

[SCSN]

Remember how "Jack of All Trades" got written in the log as "JackOfAllTrades"?

Here is how they fixed that problem:

Code: [Select]

private string replaceColor(string str)
{
    string str2 = str;
    if ((str2.Length > 5) && str2.StartsWith("-----"))
    {
        string str3 = GamePlayUtils.getColor(this.getPlayerIndex(str2.Substring(str2.IndexOf(" ") + 1), ":"), true);
        string[] textArray1 = new string[] { "<Color=", str3, ">", str2, "</Color>" };
        return string.Concat(textArray1);
    }
    str2 = this.imageText(str2).Replace("JackOfAllTrades", "Jack of All Trades");
    foreach (KeyValuePair<string, string> pair in this.pColorCards)
    {
        str2 = this.replaceListCardColor(str2, pair.Key, pair.Value);
    }
    int playerIndex = this.getPlayerIndex(str2, " ");
    string str4 = GamePlayUtils.getColor(playerIndex, true);
    if (playerIndex >= 0)
    {
        str2 = str2.Replace(GamePlayHud.i().pPlayers[playerIndex].pName + " -", "<Color=" + str4 + ">" + GamePlayHud.i().pPlayers[playerIndex].pName + "</Color> -");
    }
    return str2;
}

[werothegreat]

Have they not heard of refactoring?

[SCSN]

Coming soon:

[Kirian]

Have they not heard of refactoring?

I don't think they've even heard of just factoring.

[Schneau]

Have they not heard of refactoring?

I don't think they've even heard of just factoring.

Or re.

[rspeer]

SCSN, you're doing amazing work.

With your hackery, would it be at all possible to change the log font to something that's actually comfortably readable as running text -- y'know, like 11pt Arial* -- instead of an all-caps font that's meant for card titles?

* Helvetica. Verdana. I don't care. Some font that has never tried to be exciting.

[Witherweaver]

Have they not heard of refactoring?

I don't think they've even heard of just factoring.

Let's hope they never hear of factorializing.  We'd probably have endless recursive stacks popping up all over the place.

[Witherweaver]

SCSN, you're doing amazing work.

With your hackery, would it be at all possible to change the log font to something that's actually comfortably readable as running text -- y'know, like 11pt Arial* -- instead of an all-caps font that's meant for card titles?

* Helvetica. Verdana. I don't care. Some font that has never tried to be exciting.

*Wingdings

[SCSN]

I've been trying to figure out how to change the chat font for the last 3 days or so but it looks like it's not going to happen.

The code doesn't just say "use this font" so that you can respond with "no, I don't want that crap, use Arial". In stead, the fonts available to the application are located in a so-called .asset file, a proprietary Unity format that stores every game resource, including images and sounds. There are a few 3rd party tools available that can read this format, but only one of them seems to work with Unity 5.x files, and that thing doesn't really provide much useful information:



I have no reason to assume they've even added Arial to this file, and that long non-searchable list isn't exactly being helpful in figuring out whether they did (so far I can't find anything, and I have no way of knowing whether it hides behind one of those awful strings of random characters). This isn't really Unity's fault—I suspect they deliberately obfuscate things so that people can't easily steal resources from a game—but it does make things a lot harder.

I haven't completely given up yet, but there are some people out there for whom changing the chat font would be about 3 or 4 orders of magnitude less time-consuming than it is for me, maybe you could try asking them (and buy a lottery ticket, while you're at it).

[SCSN]

For people who already moved to 2.0.43, here is a quick patch that changes the log to make it more readable:



It doesn't yet contain any other features (though the disabling of animations is now a native feature).

[SCSN]

The regular patch for 2.0.43 is now ready, but I'm curious if there's still interest in this one or whether I can just focus exclusively on the speed version.

Right now the regular one is just the updated log + nicer Masq/Bane reveal color. Is there anyone who really likes these features but hates the high speed version?

[popsofctown]

For people who already moved to 2.0.43, here is a quick patch that changes the log to make it more readable:



It doesn't yet contain any other features (though the disabling of animations is now a native feature).

Does it find the first initial that doesn't match?