[3.4 dev] Two problems with jewelry descriptions

[PowerWyrm]

1) Ring of Light
An unidentified aware Ring of Light appears as "a Ring of Light <, +1>". Shouldn't it be "a Ring of Light <+1>" until identified?
See obj_desc_pval().

2) Randart jewelry
An unidentified aware "special" randart ring/amulet will give away its bonuses (pval, tohit, todam, toac), since showing bonuses on aware jewelry is based on fixed values of object kind... and templates for special artifacts don't have bonuses set.
See object_this_pval_is_visible(), object_attack_plusses_are_visible() and object_defence_plusses_are_visible().

[Magnate]

1) Ring of Light
An unidentified aware Ring of Light appears as "a Ring of Light <, +1>". Shouldn't it be "a Ring of Light <+1>" until identified?
See obj_desc_pval().

2) Randart jewelry
An unidentified aware "special" randart ring/amulet will give away its bonuses (pval, tohit, todam, toac), since showing bonuses on aware jewelry is based on fixed values of object kind... and templates for special artifacts don't have bonuses set.
See object_this_pval_is_visible(), object_attack_plusses_are_visible() and object_defence_plusses_are_visible().

Thanks - both these are known, and I'm pretty sure the first is fixed (perhaps only in v4, though I thought we were up to date with porting fixes back to V). Were you using the latest 3.4-dev?

The second is a longstanding problem caused by the different treatment of jewelry in the ID code. I've never fixed it because I don't believe that jewelry should be treated differently, but I've not yet got round to rewriting ID. Now that we've agreed that v4 no longer needs pseudo, it might happen.

[PowerWyrm]

The code for obj_desc_pval() still has the bug in the latest source from the "master" branch. Isn't that the main reference branch?

	for (i = 0; i < o_ptr->num_pvals; i++) {
		if (spoil || object_this_pval_is_visible(o_ptr, i)) {
			if (i > 0)
				strnfcat(buf, max, &end, ", ");
			strnfcat(buf, max, &end, "%+d", o_ptr->pval[i]);
		}
	}

Need to make a check for first "visible" pval instead of first pval.

For the second point, I think it's a trivial-to-fix bug: just mask the bonuses for unIDed aware artifact rings/amulets. Not really a pseudo-ID problem, more a description problem (special artifacts have a "template" instead of a flavor).

[Magnate]

The code for obj_desc_pval() still has the bug in the latest source from the "master" branch. Isn't that the main reference branch?

	for (i = 0; i < o_ptr->num_pvals; i++) {
		if (spoil || object_this_pval_is_visible(o_ptr, i)) {
			if (i > 0)
				strnfcat(buf, max, &end, ", ");
			strnfcat(buf, max, &end, "%+d", o_ptr->pval[i]);
		}
	}

Need to make a check for first "visible" pval instead of first pval.

But isn't that what this code snippet already does? The test is if (spoil || object_this_pval_is_visible(...)), which seems right?

For the second point, I think it's a trivial-to-fix bug: just mask the bonuses for unIDed aware artifact rings/amulets. Not really a pseudo-ID problem, more a description problem (special artifacts have a "template" instead of a flavor).

Yes, that's probably sufficiently easy to be worth doing. I'll open a ticket for it when the trac migration is finished.

[PowerWyrm]

But isn't that what this code snippet already does? The test is if (spoil || object_this_pval_is_visible(...)), which seems right?

Take the Ring of Light example: pval[0] is not visible (variable bonus to searching), pval[1] is visible (fixed bonus to light radius). The current code will skip the pval at i = 0 and add the pval at i = 1 along with a comma. So you get "a Ring of Light <, +1>".

Something like this will always work:

    bool first = TRUE;
    for (i = 0; i < o_ptr->num_pvals; i++)
    {
        if (spoil || object_this_pval_is_visible(o_ptr, i))
        {
            if (!first) strnfcat(buf, max, &end, ", ");
            strnfcat(buf, max, &end, "%+d", o_ptr->pval[i]);
            first = FALSE;
        }
    }

[Magnate]

Thank you, that's excellent. Ironically this is no longer needed in v4 (we've changed to displaying only the highest pval on the object's main line), but will be helpful for 3.4.

Opened as #1617

[PowerWyrm]

Thank you, that's excellent. Ironically this is no longer needed in v4 (we've changed to displaying only the highest pval on the object's main line), but will be helpful for 3.4.

Opened as #1617

#1617 was erased recently and been replaced by some spam...