#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)
Change History (15)
comment:1 Changed 9 years ago by Derakon
comment:2 Changed 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 years ago by Derakon
comment:10 Changed 9 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 9 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:12 Changed 8 years ago by magnate
http://angband.oook.cz/forum/showthread.php?t=4547 for more discussion.
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 8 years ago by magnate
- Resolution set to fixed
- Status changed from pending to closed
In master as of [ref5f4267]
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".