Preserve mode

[Spacebux]

My acronym decryption skills must be dwindling...

YMMV.

Yo Mama's Mental Violation ?

[PowerDiver]

That cannot be an absolute rule and has to take into account the relative costs of not fixing, quick fix and doing it right.

Generally, the cost of doing it right isn't so much, once you get into the habit. Especially in a project like this, when measuring the relative costs, you have to compare the time you spend now to the time some other person will spend debugging years from now when he modifies the code.

Given that Takkaria is spending years focused on taking out bad code to replace it with better code, I think a "first do no harm" policy is in order. If you aren't willing to do it right, IMO you should leave it on the todo list until someone is.

Each time you do it right, you make it easier for the next person to do it right. Each time you do it wrong, you make it harder [or at least less likely] for the next person to do it right. That's why I'm harping on this so much.

Hopefully, patches are coming very soon.

[Magnate]

When there are a bunch of things on the todo list, you will never get to them all, but if you are about to address an issue you should do it right instead of adding to the problem.

That cannot be an absolute rule and has to take into account the relative costs of not fixing, quick fix and doing it right.

It's not about what you record, but how you record it and access it. The property of whether an artifact can be created in the future ought to be stored in a data structure referenced by the artifact. When deciding whether to create an artifact, looping through the object list is a mistake, even if it happens to work.

Agreed. Whether an artifact can be created is already stored in o_ptr->artifact->created, and I will remove looping through o_list when I do #1014.

I just thought of a possible problem with your method. Destruction now causes artifacts to be lost. Is the code currently set up so that your solution works with that? I don't know. If things were done properly, I would know that it works. You could hack the destruction or deletion code, but that's just a sign things were implemented incorrectly in the first place.

Agreed again. When I refer to "relative costs" above, I am using that term very broadly: time cost, opportunity cost and motivational cost, to name but three. If I forced myself to fix everything that was wrong with the code related to every change I made, I would never commit anything.

Anyway, I was trying to be helpful by giving advice. It was not my goal to get on your nerves. I'll stop bothering you now. Well, I'll keep bothering you, but not about this.

Thanks. I am always happy to receive advice, but patches are even more welcome!

[PowerDiver]

This seems to me a lot like "recording whether an object has ever been on the floor", albeit only for artifacts. I disagree that it is necessary to implement this now - that would be taking the "you know will not extend" principle to an absurd extreme.

When there are a bunch of things on the todo list, you will never get to them all, but if you are about to address an issue you should do it right instead of adding to the problem.

It's not about what you record, but how you record it and access it. The property of whether an artifact can be created in the future ought to be stored in a data structure referenced by the artifact. When deciding whether to create an artifact, looping through the object list is a mistake, even if it happens to work.

I just thought of a possible problem with your method. Destruction now causes artifacts to be lost. Is the code currently set up so that your solution works with that? I don't know. If things were done properly, I would know that it works. You could hack the destruction or deletion code, but that's just a sign things were implemented incorrectly in the first place.

Anyway, I was trying to be helpful by giving advice. It was not my goal to get on your nerves. I'll stop bothering you now. Well, I'll keep bothering you, but not about this.

[Magnate]

The old code required looping through all objects to check if the artifact is present, when deciding whether to allow an artifact to be created. I don't know if that has been changed, but the obvious method is an array of what not to create on the current level. Once you are doing that, it is natural to go to the two array approach.

I stand by the principle that you shouldn't write even a single line of code that you know will not extend, and that goes triple when the extension seems likely. YMMV.

I do not disagree with the principle, but you seem to have misunderstood either how preserve off works or what I am changing. You seem to be talking about object creation, when I am talking about one extra check in wipe_o_list(), for whether the object is held by a monster (exactly as Timo describes).

EDIT:
Ok, I think I finally worked out why you are talking about object creation. As usual we are at completely cross purposes, and you have taken this as a basis for disagreement. Though I have to say in this case I do actually disagree (if I have understood you correctly - the phrase "check if the artifact is present" seems imprecise if I have).

I think you are saying that instead of checking for artifact->created at creation time, you should check both artifact->can_be_created_at_all and artifact->can_be_created_on_this_level, i.e. two separate bools. The second would be set to FALSE once an artifact is generated in a monster inventory, and the first would be set once it hits the floor (if preserve is off), or once it is known.

This seems to me a lot like "recording whether an object has ever been on the floor", albeit only for artifacts. I disagree that it is necessary to implement this now - that would be taking the "you know will not extend" principle to an absurd extreme. We know that we may one day use rune-based ID, yet there are ten dozen things we have changed without making them extensible for that.

[Timo Pietilä]

Well, I might argue that instead, preserve mode off should be eliminated as an option.

But if that's not going to happen, I think the proposed solution would be fine. If monsters are allowed to pickup artifacts in the future, that bridge can be crossed at a later date.

Preserve off monster inventories require only one additional check in event of leaving level: is it into monster inventory? Other check is is it identified (IE. has it been found)? If latter it is lost either way, if not then check if it is in inventory and preserve it.

Initial state of the artifact doesn't matter in either case, except for creating level feeling. If there is a artifact in level, monster carrying or not, it should trigger special feeling in preserve off just in case.

[PowerDiver]

It is true that this change will make preserve off less challenging if monsters can pick up unIDd artifacts from the floor, because those artifacts will be preserved when they should be lost - but I'm content to require an additional fix for that if such a change is made to monster pickup. It would require recording whether an object has ever been on the floor, and that seems a pointless thing to add before it is required.

My suggestion does not require recording that on the object. It is implicit in whether to allow artifact creation.

The old code required looping through all objects to check if the artifact is present, when deciding whether to allow an artifact to be created. I don't know if that has been changed, but the obvious method is an array of what not to create on the current level. Once you are doing that, it is natural to go to the two array approach.

I stand by the principle that you shouldn't write even a single line of code that you know will not extend, and that goes triple when the extension seems likely. YMMV.

[camlost]

Well, I might argue that instead, preserve mode off should be eliminated as an option.

But if that's not going to happen, I think the proposed solution would be fine. If monsters are allowed to pickup artifacts in the future, that bridge can be crossed at a later date.

[Magnate]

I have no interest in preserve off, so the behaviour does not matter to me, but I want to point out that some time in the future some maintainer may decide that monsters capable of dropping artifacts should be capable of picking them up. I don't think you should write a solution that depends upon no pickup.

It's one thing to leave code alone because it works for now. It's another to write new code with the same problems.

The whole loop through objects on level change is inelegant. Would it be any more work to maintain two arrays, do_not_create_artifact_again_on_this_level[] and do_not_create_artifact_again[]? Set the second on item creation for preserve on, and set the second only when an item hits the floor for preserve off.

Well, I'm not actually writing any new code, other than adding one additional limb to a single if statement to check for o_ptr->held_m_idx. The entire o_list concept is screwy anyway, because it ought to be a linked list and not an array, but that's a lot of work to change and needs to be considered separately from this fix. Finally, if someone does allow monsters to pick up artifacts, then the concept of preserve off needs re-thinking anyway.

All I'm doing is making the behaviour of preserve off the same as it was before monsters carried their drops. It is true that this change will make preserve off less challenging if monsters can pick up unIDd artifacts from the floor, because those artifacts will be preserved when they should be lost - but I'm content to require an additional fix for that if such a change is made to monster pickup. It would require recording whether an object has ever been on the floor, and that seems a pointless thing to add before it is required.

[PowerDiver]

My suggestion is that artifacts carried by monsters should be preserved even if preserve is off. Since monsters cannot pick up artifacts from the floor, this will only apply to the drops generated at monster creation time, and will therefore bring us back to exactly the previous behaviour of preserve off.

Anyone want to argue for a different behaviour?

I have no interest in preserve off, so the behaviour does not matter to me, but I want to point out that some time in the future some maintainer may decide that monsters capable of dropping artifacts should be capable of picking them up. I don't think you should write a solution that depends upon no pickup.

It's one thing to leave code alone because it works for now. It's another to write new code with the same problems.

The whole loop through objects on level change is inelegant. Would it be any more work to maintain two arrays, do_not_create_artifact_again_on_this_level[] and do_not_create_artifact_again[]? Set the second on item creation for preserve on, and set the second only when an item hits the floor for preserve off.

[Timo Pietilä]

Before 3.1.x that's true, but since then lost artifacts should show up in the knowledge menus after fleeing a special level.

Yes, but because of the bug there never were lost artifacts, so you never knew if you just got lucky and didn't lose an artifact. That prevented you from noticing the bug.

[Magnate]

One thing to cause that illusion is that you never know if there actually were artifact in the level, so in my games I just thought that I have been lucky in those rare instances where I needed to get away from special level without checking everything.

Before 3.1.x that's true, but since then lost artifacts should show up in the knowledge menus after fleeing a special level.

[Magnate]

That solution sounds good to me.

How would artifacts carried by monsters affect level feelings?

At the moment they would boost level feelings (and cause "special" feelings in preserve off mode) ... but level feelings are due for a revamp any year now ...

[Timo Pietilä]

Ok, so the latest autobuild development version (formerly known as "nightly") effectively breaks Preserve-off mode, since artifacts generated as monster drops will be lost if the player leaves the level.

This is only an issue for people playing with Preserve off, and given that nobody noticed the previous bug with it for over two years, we can conclude that this is a very small proportion of people.

One thing to cause that illusion is that you never know if there actually were artifact in the level, so in my games I just thought that I have been lucky in those rare instances where I needed to get away from special level without checking everything.

There was other bug with "8" -block vaults that didn't force them to have excellent items, and I knew it, so I didn't even expect to see artifacts in vaults.

[Netbrian]

That solution sounds good to me.

How would artifacts carried by monsters affect level feelings?