r1676 - Random Ability Bug on Ego

[Sirridan]

When getting egos, (Blessed), Lothlorien, and Magi, I seem to only get slow metabolism as the random ability, and random resists on gear (Aman / Preservation) give rPoison. Not that I mind the rpoison, I always seem to lack it... but yeah.

2 aman, 1 preservation both had rPoison, and at least 30 random ability egos or more were all slow metabolism.

Unless this was found and fixed in a later version?

EDIT:

Checked out the code, seems that rPoison and Slow Digest are both the first element of their ego_xtra arrays... is anything wrong with get_new_attr?

u32b get_new_attr(u32b flags, const u32b attrs[])
{
	size_t i;
	int options = 0;
	u32b flag = 0;
	for (i = 0; i < N_ELEMENTS(attrs); i++)
	{
		/* skip this one if the flag is already present */
		if (flags & attrs[i]) continue;

		/* each time we find a new possible option, we have a 1-in-N chance of
		 * choosing it and an (N-1)-in-N chance of keeping a previous one */
		if (one_in_(++options)) flag = attrs[i];
	}
	return flag;
}

looking at this, it should be 1 in 1 for poison, then 1 in 2 to change it to the next, and so on... but it always chooses poison. The code segment looks fine to me, unless one_in_(x) was changed recently?

More edits:

Also noticed for random sustains, it always picks strength, which is also first in the sustain list...

EVEN MORE EDITS OMG: One more elvenkind with rPoison...

[PowerDiver]

I think the bug is with N_ELEMENTS. That should only be used on an array[size], not on *array, even though they appear to be the same type. I think the N_ELEMENTS is always returning 1, so it doesn't get past the first element.

[Sirridan]

I think the bug is with N_ELEMENTS. That should only be used on an array[size], not on *array, even though they appear to be the same type. I think the N_ELEMENTS is always returning 1, so it doesn't get past the first element.

Sounds like that would be it, because I just found another magi with slow dig... *sigh* maybe one of those elvenkinds would have had the resist holes my latest char has... oh well.

The nearly guaranteed rPoison was nice for a bit though

[Sirridan]

Actually, I ran this code segment on the same basic array, and N_ELEMENTS was returning 12 for resists as it should...

#include <stdio.h>

#define N_ELEMENTS(a) (sizeof(a) / sizeof((a)[0]))

#define	        TR1_RES_POIS	0x1
#define	        TR1_RES_FEAR	0x2
#define	        TR1_RES_LITE	0x4
#define	        TR1_RES_DARK	0x8
#define	        TR1_RES_BLIND	0x10
#define	        TR1_RES_CONFU	0x20
#define	        TR1_RES_SOUND	0x40
#define	        TR1_RES_SHARD	0x80
#define	        TR1_RES_NEXUS	0x100
#define	        TR1_RES_NETHR	0x200
#define	        TR1_RES_CHAOS	0x400
#define	        TR1_RES_DISEN	0x800


void main()
{
	static const unsigned int ego_resists[] =
	{
	        TR1_RES_POIS,
	        TR1_RES_FEAR,
	        TR1_RES_LITE,
	        TR1_RES_DARK,
	        TR1_RES_BLIND,
	        TR1_RES_CONFU,
	        TR1_RES_SOUND,
	        TR1_RES_SHARD,
	        TR1_RES_NEXUS,
	        TR1_RES_NETHR,
	        TR1_RES_CHAOS,
	        TR1_RES_DISEN,
	};
	printf("%d\n",N_ELEMENTS(ego_resists));
	printf("done\n");
}

Any thoughts?

[zaimoni]

Actually, I ran this code segment on the same basic array, and N_ELEMENTS was returning 12 for resists as it should...

Which is completely irrelevant to the original bug.

#include <stdio.h>

#define N_ELEMENTS(a) (sizeof(a) / sizeof((a)[0]))

/* ... */

void main()
{
	static const unsigned int ego_resists[] =
	{
	        TR1_RES_POIS,
	        TR1_RES_FEAR,
	        TR1_RES_LITE,
	        TR1_RES_DARK,
	        TR1_RES_BLIND,
	        TR1_RES_CONFU,
	        TR1_RES_SOUND,
	        TR1_RES_SHARD,
	        TR1_RES_NEXUS,
	        TR1_RES_NETHR,
	        TR1_RES_CHAOS,
	        TR1_RES_DISEN,
	};
	printf("%d\n",N_ELEMENTS(ego_resists));
	printf("done\n");
}

The extent of the ego_resists array is calculated at compile time, and happens to be 12. While in the original code:

u32b get_new_attr(u32b flags, const u32b attrs[])
{
	size_t i;
	int options = 0;
	u32b flag = 0;
	for (i = 0; i < N_ELEMENTS(attrs); i++)
	{
		/* skip this one if the flag is already present */
		if (flags & attrs[i]) continue;

		/* each time we find a new possible option, we have a 1-in-N chance of
		 * choosing it and an (N-1)-in-N chance of keeping a previous one */
		if (one_in_(++options)) flag = attrs[i];
	}
	return flag;
}

The parameter const u32b attrs[] is decayed at compile time to const u32b *attrs . The language standards (C90/C99/C1X) require 1==N_ELEMENTS(attrs).

The most direct fix is

u32b get_new_attr(u32b flags, const u32b *attrs, u32b attrs_size)
{
	size_t i;
	int options = 0;
	u32b flag = 0;
	for (i = 0; i < attrs_size; i++)
	{
		/* skip this one if the flag is already present */
		if (flags & attrs[i]) continue;

		/* each time we find a new possible option, we have a 1-in-N chance of
		 * choosing it and an (N-1)-in-N chance of keeping a previous one */
		if (one_in_(++options)) flag = attrs[i];
	}
	return flag;
}

and use N_ELEMENTS on the array where it actually is an array.

[Sirridan]

Which is completely irrelevant to the original bug.

The extent of the ego_resists array is calculated at compile time, and happens to be 12. While in the original code:

u32b get_new_attr(u32b flags, const u32b attrs[])
{
	size_t i;
	int options = 0;
	u32b flag = 0;
	for (i = 0; i < N_ELEMENTS(attrs); i++)
	{
		/* skip this one if the flag is already present */
		if (flags & attrs[i]) continue;

		/* each time we find a new possible option, we have a 1-in-N chance of
		 * choosing it and an (N-1)-in-N chance of keeping a previous one */
		if (one_in_(++options)) flag = attrs[i];
	}
	return flag;
}

The parameter const u32b attrs[] is decayed at compile time to const u32b *attrs . The language standards (C90/C99/C1X) require 1==N_ELEMENTS(attrs).

The most direct fix is

u32b get_new_attr(u32b flags, const u32b *attrs, u32b attrs_size)
{
	size_t i;
	int options = 0;
	u32b flag = 0;
	for (i = 0; i < attrs_size; i++)
	{
		/* skip this one if the flag is already present */
		if (flags & attrs[i]) continue;

		/* each time we find a new possible option, we have a 1-in-N chance of
		 * choosing it and an (N-1)-in-N chance of keeping a previous one */
		if (one_in_(++options)) flag = attrs[i];
	}
	return flag;
}

and use N_ELEMENTS on the array where it actually is an array.

Ah forgive my ignorance, makes sense, now I know more of what PowerDiver was saying as well.

[Magnate]

Now trac'd as http://trac.rephial.org/ticket/969 - thanks for the report.

[d_m]

Should be fixed in r1678. Thanks for finding the bug.

[zaimoni]

The parameter const u32b attrs[] is decayed at compile time to const u32b *attrs . The language standards (C90/C99/C1X) require 1==N_ELEMENTS(attrs).

Decay happens, but the equality is wrong. The result does have nothing to do with the original size of the static array, but is rather likely to be a small integer as the actual expression ends up being equal to sizeof(u32b*)/sizeof(u32b).

The most likely values on real platforms (8-bit bytes) would be:
0: 64-bit pointers (e.g., native Win64 build)
1: 32-bit pointers (e.g., native Win32 build)
2: 16-bit pointers (if non-extended DOS build were resurrected)

[Sirridan]

Is it cheating to play a character for a bit until I find an elvenkind or Aman to guarantee early rPoison, then switch over to the new version?

[d_m]

Is it cheating to play a character for a bit until I find an elvenkind or Aman to guarantee early rPoison, then switch over to the new version?

Eh, if you do this once I think it's probably fine.

Reverting to the old version each time you want poison resistance might be. I dunno, I don't lose a whole lot of sleep about edge cases like this.

[Bill Peterson]

Is it cheating to play a character for a bit until I find an elvenkind or Aman to guarantee early rPoison, then switch over to the new version?

Just make sure you let everyone know about it if you post a YAWP or to the ladder.