Minor improvment

[obligaron]

Before this PR we were generating items with CF_LEVEL > 45.
This was no problem, but now we check this in IsDungeonItemValid.

With this PR we only generate items with a level that a monster has. This ensures that we can use these items when testing multiplayer.
I discovered this when generating the doppelganger items for #6890.

[StephenCWills]

KP considered this quirk to be a feature, since debug items would have a harder time masquerading as legitimate drops. Though, as this PR demonstrates, it's a pretty weak defense.

[obligaron]

Obscurity != security. 😁

My main driver is testing in multiplayer. Especially if special items are needed.

Also the current code can generate total valid items. But not always. That means even people without knowledge only have to run the function often enough or? 🤔

Some alternatives are

• settings a special flag (so at least we know it's generated by the debug code)
• don't use item checks in debug build or provide a switch to turn them off
• something else

[AJenbo]

Turn off the check in debug makes sens. Makes it harder to send them to someone who isn't aware of there origin

[StephenCWills]

Obscurity != security. 😁

I agree, of course, but this isn't really a matter of security in the first place considering the way multiplayer is designed.

Also the current code can generate total valid items. But not always. That means even people without knowledge only have to run the function often enough or? 🤔

To get a valid item, a person who does not explicitly modify the code to generate valid items would instead need to verify the validity of each item they generate. Depending on their level of skill/knowledge, this could end up being a fairly prohibitively laborious and error-prone process. The feature justifies itself as a hurdle as opposed to a security mechanism.

[StephenCWills]

Turn off the check in debug makes sens. Makes it harder to send them to someone who isn't aware of there origin

I like this, but we also should be able to test the validation in debug mode. So maybe a separate precompiler flag to explicitly disable the validation?

[obligaron]

I like this, but we should also be able to test the validation in debug mode. So maybe a separate precompiler flag to explicitly disable validation?

If you want to raise the bar, it should be disabled by default in debug, and perhaps have a separate flag to enable it in debug.

To get a valid item, a person who is not explicitly modifying the code to generate valid items would instead have to check the validity of each item they generate. Depending on their level of skill/knowledge, this could end up being a rather prohibitively tedious and error-prone process. The feature is justified as a hurdle rather than a security mechanism.

I'm not sure it's a big hurdle. The logic of how the check is done is clean enough that it's fairly easy to understand, especially when it's enabled in debug mode. 😉
On the other hand, it makes the life of every other developer more error-prone, as you'll never have validation while debugging, or be able to generate items for testing without changing some configuration.

Personally, I would just generate valid items. But I don't have a strong motivation/interest in that either, so I'm fine with whatever we decide.

[StephenCWills]

If you want to raise the bar, it should be disabled by default in debug, and perhaps have a separate flag to enable it in debug.

I'm not sure what you mean by "raise the bar" in this context, but I did consider that, and it has its pros and cons. Ultimately, the reason I decided to suggest enabling it by default is because a developer who encounters a validation error is going to work to try and solve it. A developer who encounters no resistance at all will be none the wiser.

I'm not sure it's a big hurdle.

Yeah, and I suppose it's been made that much smaller by the existence of this PR. But I don't think it has to be a big hurdle. The point is that someone who is not that confident in their ability to read or write C++ code might attack the problem differently than you or I, ultimately deciding that clearing the hurdle is not worth the hassle.

EDIT: Ah, I think I just realized what "raise the bar" meant. You meant it would make the hurdle bigger, right?

[obligaron]

I'm not sure what you mean by "raise the bar" in this context, but I did consider that, and it has its pros and cons. Ultimately, the reason I decided to suggest enabling it by default is because a developer who encounters a validation error is going to work to try and solve it. A developer who encounters no resistance at all will be none the wiser.

Yeah but on the other side it makes multiplayer testing harder if you need special items (like Doppelganger). That's why I meant you can't have both at the same time. 😉

Yeah, and I suppose it's been made that much smaller by the existence of this PR.

Sorry for that. I didn't know the current behavior was intentional.
Perhaps some comment in drop command could help that no one else try to change how generating item works.

EDIT: Ah, I think I just realized what "raise the bar" meant. You meant it would make the hurdle bigger, right?

Yes I meant it would make it harder, because in your debug build it works but for other player it don't work. That would make it harder to diagnostic.