Strange behaviour when giving messages about god sacrifices

[DivineHammer]

I've been looking at the code in godprayer.cc and noticed something strange. If you sacrifice a corpse/item to a god like Nemelex or Jiyva, and that item has, say, a 50% chance of giving 1 piety, then the game:

- flips a coin to decide whether or not you get the piety
- flips another coin to set relative_piety_gain, which determines what kind of message the player sees ("the corpse slowly burns to ash" vs. "the corpse is consumed by flame" etc.).

In other words, it's possible to gain piety and get the negative message, or to not gain piety but get the positive message. Is this an intentional design decision? When I saw it, I found it quite shocking, since it contradicts how I interpreted those sacrifice messages as a player. It seems awfully misleading (though, admittedly, not terribly harmful).

It would seem logical to me to have gain_piety return the amount of piety gained and then base the message to the player on that value.

While I'm here, a small unrelated observation: what's up with line 1649 in shopping.cc in the function item_value? As far as I can tell, both fresh and rotten chunks are going to fall through and have their value set to 1, so why have the if statement checking whether the chunk is rotted?

(By the way, sorry if this is the wrong subforum - feel free to move it if it is)

[The Mantis]

For reference:

  Code:

case FOOD_CHUNK:

1650    

            if (food_is_rotten(item))

1651    

                break;

Which means, "IN the case: FOOD_CHUNK

If FOOD_IS_ROTTEN attaches AN ITEM

BREAK LINE OF COMMAND."

In the context of shopping.cc, this means that if an item, that item being some sort of food, has a property, that property being 'rotten', then the command ends without statement.

[DivineHammer]

Ah, okay, I think I get it now. So rotten chunks get a value of 1 and fresh chunks get a value of 4 because they reach the assignment on line 1657 along with Chokos, Lychees, etc.? I sometimes get mixed up with the falling-through behaviour in switch statements, since my first language is Python.

If it were up to me, I would get rid of the if and replace it with:

  Code:

valued = food_is_rotten(item) ? 1 : 4;
break;

But probably the code as it exists is perfectly straightforward and readable to a C++ programmer.

[galehar]

I've been looking at the code in godprayer.cc and noticed something strange

Don't know. This part of the code is quite messy indeed.

But probably the code as it exists is perfectly straightforward and readable to a C++ programmer.

Your version is certainly easier to read. But I think the point is to have the food value defined in only one place. With your version, if someone changes the 4 to something else he might not notice that the value is duplicated above.

[DivineHammer]

Do you mean you're not sure whether or not the code is behaving as I described or you're not sure whether or not the behaviour is intentional? If it's the former, I can point you to where specifically in the code this is happening (or seems to be happening). For example, line 578 in godprayer.cc:

  Code:

gain_piety(gain, 700);
gain = div_rand_round(gain, 700);
return (gain <= 0) ? PIETY_NONE : (gain < 4) ? PIETY_SOME : PIETY_LOTS;

If you think it's worthwhile, I could open a ticket (and possibly try to write a patch). Like I said, I'm just not sure whether or not it's an intentional design decision.

[galehar]

But there's no randomization there. Or are you talking about div_rand_round? It's not really a randomizer, it's just an integer division that randomly increments the result depending on the rest, to have a more accurate average.

[DivineHammer]

Well gain_piety is also random (as long as the numerator isn't a multiple of the denominator). I don't know what values gain can reasonably be expected to range over, but let's say it's 350 in this case. Then gain_piety(gain, 700) will be equally likely to increase the player's piety by 1 or leave it alone. Then div_rand_round(gain, 700) will return 0 or 1 with equal chance (independent of the previous roll) and gain will be set to that value. So the player might get no piety, but get a PIETY_SOME message, or they might get piety while getting a PIETY_NONE message.

It seems like, in practice, when you're sacrificing corpses and items, you're dealing with the case where 0 < expected piety gain < 1, so I would expect this issue to manifest itself often.

[galehar]

OK, I see what you mean. It would indeed be very easy to fix by simply making gain_piety return the actual gain, as you suggested. In practise, it would be almost unnoticeable, because the piety scale is so opaque. At least, new god powers will only be gained when an actual piety gain message would be printed. It's minor, but it's still an improvement. You can submit a patch if it bothers you.

[Kate]

I had a vague feeling that this was intentional behaviour, to not show you exactly when you're gaining piety but to still give you an indication of what percentage of the time piety gain happens. I might be completely wrong though, and it's still a bit weird (piety gain from corpses could do with changing anyway though, really, since it makes Hive/Orc even worse as sources of free piety).

[DivineHammer]

I found the answer to my question digging through the commit log for godprayer.cc. Messages used to reflect actual piety gain until a patch from Kilobyte about a year ago:

commit b69cef0dc5e8357407ed4378bd742ac448683286
Author: Adam Borowski <kilobyte@angband.pl>
Date: Wed Jul 7 14:21:02 2010 +0200

Smooth the remaining piety gains. Decouple the messages from actual piety

I still think this is an odd choice, since it misleads the player. However, I am happy to have gotten to the bottom of this mystery.