Opened 8 years ago

Closed 7 years ago

Last modified 8 years ago

#1394 closed change (fixed (in master))

Allow ego_min_pval to be zero

Reported by: magnate Owned by: magnate
Milestone: 3.3.0 Keywords: items
Cc: elly

Description

Important for mixed-blessing items. Derakon discovered that you can't put negative pval ranges in the current ego template:

http://angband.oook.cz/forum/showthread.php?p=51066#post51066

Attachments (1)

comments_diff.txt (23.6 KB) - added by Derakon 8 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by Derakon

Oddly enough, I was able to make standard objects with negative pvals (e.g. a ring of CON that reduced INT) without trouble, so I suspect it's the "minimum pval" field in the ego definition that's causing trouble.

Also, the parser doesn't like formats like "-1-M5", complaining that they're "not random".

comment:2 Changed 8 years ago by magnate

  • Owner set to magnate
  • Status changed from new to assigned

Maybe - were your CON rings fixed or random in their negative INT pval? The problem may be that we can't deal with a negative random_value. That would be more difficult than fixing the min pval issue.

comment:3 Changed 8 years ago by Derakon

Uh, I don't remember, and I don't have the game handy to check right now. Sorry.

comment:4 Changed 8 years ago by magnate

  • Milestone changed from 3.3.0 to Future
  • Status changed from assigned to confirmed

Punting in accordance with new milestone policy (that any other milestone is only set once someone is actually working on the ticket).

comment:5 Changed 8 years ago by Derakon

I poked at this a bit; the problem is that the min_pval field is getting set erroneously, in parse_e_l. The call to parser_getint(p, "min") returns a massive value, which is then added on at ego creation time. The parser code is horribly hairy for me (who names a function "parse_e_l" and then doesn't comment on what it does?) so I don't really know how to go about fixing it, but that's the place to start.

comment:6 Changed 8 years ago by takkaria

  • Cc elly added
  • Milestone changed from Future to 3.3.0
  • Type changed from change to bug

Re-assigning to 3.3 and CCing Elly because this is a parser bug (see above comment).

comment:7 Changed 8 years ago by elly

This is not a parser bug.

object/object.h: byte min_pval[MAX_PVALS]; /* Minimum pval */

-1 in two's complement is 0xFFFFFFFF, which (when coerced down to a byte) is 0xFF, which is 255. If you want signed minima, you have to store them in a signed type. I believe Magnate is undertaking some clever hack to fix this.

(Our not accepting "-1-M5" as a pval expression is a separate problem, to do with the parser's refusal to accept random values with more than one minus sign. I am considering how to fix it.

comment:8 Changed 8 years ago by elly

By the way, the function names: parse_X_Y() parses Y: lines for X_info, so parse_e_l parses L: lines for e_info. I couldn't really think of a nicer way to name these.

comment:9 Changed 8 years ago by magnate

  • Status changed from confirmed to pending

This is fixed in [r821f59d] by ensuring that a min_pval of 0 is not actioned. So go right ahead and put those negative pvals into ego_item.txt - just stick to either fixed numbers (-X) or random (-XdYMZ), but not both. See #1451 for the ticket to fix this.

Changed 8 years ago by Derakon

comment:10 Changed 8 years ago by Derakon

Awesome, thank you!

Elly: that makes sense, now that I know what it is. The problem was that I didn't know what it was when I saw it. That in mind, a bit of hairy vim search&replace has generated this diff that adds a simple comment to the top of each parse_foo_bar function. Next time someone who isn't use dives into this code, they'll need to spend a little less time figuring out where they are. :)

comment:11 Changed 8 years ago by magnate

  • Milestone changed from 3.3.0 to 3.4.0
  • Status changed from pending to confirmed
  • Summary changed from Allow ego types to have negative pvals to Allow ego_min_pval to be zero
  • Type changed from bug to change

Ok, I'm taking this back out of pending, as once #1451 is fixed we will need to allow 0 to be a genuine minimum pval. So the obvious solution is to use 255 as the value that means "do not specify a minimum", but there may be a better solution. We may also want to specify a minimum which is below zero (e.g. for a pval of -3+M8 you might want a floor of -2), so we could change min_pval[] from byte to s16b - but we ought to have a "no minimum" even if we do that.

comment:13 Changed 8 years ago by magnate

  • Milestone changed from 3.4.0 to 3.3.0
  • Status changed from confirmed to pending

[r40d63b8]

comment:14 Changed 7 years ago by magnate

  • Resolution set to fixed
  • Status changed from pending to closed

In master as of [ref5f4267]

Note: See TracTickets for help on using tickets.