Tweet
I'm experiencing an odd bug in the development versions - after updating to the last one, objects I find do not stack anymore with those I had before the switch. I'm now carrying two separate packs of CLW and CSW, for instance. Could it be due to the recent changes in object descriptions?
-
--
Dive fast, die young, leave a high-CHA corpse. -
I bet that once you drink all the "old" potions this won't happen.
We've seen these kinds of bugs with old savefiles while developing. I would recommend that people finish their games before trying the newest nightly, although (hopefully) other than minor stacking bugs like this they should be compatible.
Thanks for reporting this! Feel free to open a bug, but if this only happens with an old savefile then I expect it will be lower priority than more serious bugs. -
The effects have changed. It is possible that your old potions have the old effects and the new potions have the new effects and so they are in fact different despite having the same name. Of course, I'm just guessing.
[edit] Never mind. The screen dump in the other thread shows restore life not stacking, and that hasn't changed to my knowledge.Comment
-
It seems the problem effects *everything*.
Does anyone want to buy c.200 assorted Magic Books, seven Mushrooms of Vigor, twenty or so miscellaneous potions of a healing-related nature, and a dozen Banishments, Mass Banishments, and Destructions?
***
Quick aside: 'Vigor' is inconsistent with the English English used elsewhere.Last edited by scud; May 6, 2011, 07:27.Comment
-
No, this is because the object flags for pre-existing objects are not set correctly when the savefile is loaded. If anyone can see what's wrong with rd_item_2() in load.c, I'd appreciate understanding where I messed up. As far as I can see, we give each object its ->base and ->kind flags, and that ought to make them stack with new ones, but somehow they are not getting some flags somewhere."Been away so long I hardly knew the place, gee it's good to be back home" - The BeatlesComment
-
Perhaps the kind flags are not yet initialized when rd_item_2 is read. I didn't bother checking.
The real problem is direct access of o_ptr->flags. Everything should go through object_flags(.,.) instead. It is bad style to copy flags from kind to object. If you folks are insistent on doing that, expect a bunch of problems both now while you work out the details and later when you're not expecting it.
I suggest you apply the attached patch at least for now. I didn't test it much, but IMO it is the right solution and any problems it causes I'd call preexisting bugs.Comment
-
I just checked the old code, and it appears that object_flags used to have the stuff I put in and it was purposely removed. That makes my suggested fix pretty pointless.Comment
-
This isn't an area of the code I have spent a lot of time in so far.
Can you explain your reasoning? I get that you think that o_ptr->flags shouldn't be used directly because you don't want it to contain kind->flags and kind->base->flags. What's the reason for that? Like I said, I haven't messed around much with ID/item generation/etc so I don't have a lot of intuition here.
One nice thing I can see about making o_ptr->flags authoritative is that you can reduce the number of lines of code you need to write to look at object flags. That said, I am in favor of adding lots of helper methods like "obj_grants_telepathy" anyway, so maybe it's not such a big deal for me.
For instance, p_ptr->state functions this way now--it is authoritative for the player and whenever items are removed/worn or status effects start/end it is recalculated. Instead of creating player_state every time we need it, we can just use p_ptr->state.Comment
-
Hierarchies should be hierarchies. If you copy something from one level to another, you are just messing things up.
Imagine a new spell that changes a potion into a staff. If you implement hierarchically, such a thing is as simple as changing the object kind and setting the number of charges. If you throw away the hierarchy, then you can't do it without a lot of work. That's not to say anyone would ever do this. This is just an example of the power of the approach.
Magnate's current attempt at a fix in rd_item_2 will only work if you only add flags. If at some point a flag is subtracted from an object kind or base, things will stop stacking again.
Data structures change. If you access everything through functions, then changing a data structure can be restricted to a single file that is responsible for the access functions to the data structure. If you allow direct access, upgrading a data structure is a mess requiring you to look at the entire project.
You could imagine implementing a flag that is only active during nighttime. Or a curse that negates a random flag for a while. If everyone calls object_flags, you just add the code there to check the gameturn. That's much cleaner than removing and replacing flags on the item. If you allow direct access, you have to propagate the special casing throughout the code, and bugs will crop up if someone misses something.Comment
-
In other words, by going through a function you can implement proper data hiding. If objects were (proper C++) classes, then their properties would be private fields with getter and setter functions, and as long as the interface didn't change you could do whatever you liked behind the scenes.
It's certainly possible to write effective, maintainable code without using data hiding, but it's a lot easier to write bad code too.Comment
-
I undersatnd the arguments about encapsulation (data hiding) and in fact like I said I don't want the code to directly access these data structures at all (and rather have functions like player_has_see_invis() or something). Elly and I have both been pushing for more and more encapsulation in Angband.
That said, there is still a question of when things like players, monsters, items, dungeons, etc should save intermediate values (their current state, their current flags, etc) and when they shouldn't. I feel like it's fair for items (like the player object and the dungeon features object) to save their current state, and put the burden of keeping it consistent on an update function (like calc_bonus() is for the player).
This way if someone wants to polymorph a sword into a hat they can do it, but they need to call an update function. In fact, if you think about it, there' s a lot of other checks/fixups that need to happen (you can't wield a hat as a weapon, for instance, and you might need to drop something if your pack if full, etc, etc).Comment
-
takkaria whispers something about options. -more-Comment
Comment