Fix torn notes not combining when moved into inventory

[morfidon]

Fixes #8487

Torn notes were only combined when picked up from the ground. The
combination logic is currently triggered through CheckQuestItem(),
which runs during ground pickup (InvGetItem / AutoGetItem).

Because of that, when notes are moved into the inventory through
other flows (for example from stash transfers), the combine logic
never runs and the notes remain separate.

This change introduces a small helper that runs when an item is added
to the inventory. It checks whether all three torn notes are present
and performs the Na-Krul note combination there.

The helper is executed from the inventory insertion paths so the
behavior is consistent regardless of how the notes enter the
inventory.

Special care was taken to avoid referencing stale inventory entries,
since the combination removes items and compacts the inventory.

Scenarios to test (I don't have notes - @Ruler2112 could you test it out?)

• picking up the notes from the ground (original behavior preserved)
• moving notes from stash into inventory
• transferring notes directly into inventory

[qndel]

I don't understand, you added a new function but never call it?

[morfidon]

I don't understand, you added a new function but never call it?

Sorry I didn't push one local change got a bit weird behavior on Windows I'm trying to fight with ^^

[yuripourre]

@morfidon does this logic can be called without Hellfire installed?

[morfidon]

@morfidon does this logic can be called without Hellfire installed?

Yes, the function can be reached from shared inventory insertion paths even without Hellfire, since ChangeInvItem() / AutoPlaceItemInInventory() are not Hellfire-only.

That said, it should be effectively a no-op there, because the helper returns immediately unless the inserted item is one of IDI_NOTE1/2/3 and all three torn notes are present.

So the current behavior is: callable in vanilla paths, but inert unless Hellfire torn-note items are actually involved.

If you prefer, I can add an explicit if (!gbIsHellfire) return ...; guard at the top to make that intent clearer.

[morfidon]

Looks like this failed in the macOS packaging step, not in compilation.

The log shows CPack failing during DMG creation with:

hdiutil: create failed - Resource busy

So this seems more like a transient macOS runner / packaging issue than a failure caused by the code in this PR. A rerun of the job is probably worth trying first.

[Ruler2112]

I'm sorry @morfidon - I kept playing & also do not have the notes anymore. I'm starting the Crypts in Hell difficulty, but do not know if I'm going to make it through due to the spike in difficulty referenced in a closed idea I had. (Seems like each enemy hit started duplicating 3-6 times based on the number of armor tinks I hear for each attack animation when I entered the caves.) If I do manage to complete the crypts and obtain all 3 notes, I will test them combining when obtaining in different ways.

[yuripourre]

@morfidon does this logic can be called without Hellfire installed?

Yes, the function can be reached from shared inventory insertion paths even without Hellfire, since ChangeInvItem() / AutoPlaceItemInInventory() are not Hellfire-only.

That said, it should be effectively a no-op there, because the helper returns immediately unless the inserted item is one of IDI_NOTE1/2/3 and all three torn notes are present.

So the current behavior is: callable in vanilla paths, but inert unless Hellfire torn-note items are actually involved.

If you prefer, I can add an explicit if (!gbIsHellfire) return ...; guard at the top to make that intent clearer.

Okay, in that case they way you did is fine. I was just wondering what would happen if an enum was missing or the Hellfire mod was off.

Another point: at first sight seems that a lot was changed just to check for new items in inventory. I am planning to review later this week but feels like it could be simplified, I can be wrong.

[StephenCWills]

Another point: at first sight seems that a lot was changed just to check for new items in inventory. I am planning to review later this week but feels like it could be simplified, I can be wrong.

This is a really good point. For instance, why do we need a whole new function called TryCombineInsertedNaKrulNote() when we already have TryCombineNaKrulNotes()? The functionality has to be similar between these two functions. Can they not be combined in some way?

[morfidon]

Another point: at first sight seems that a lot was changed just to check for new items in inventory. I am planning to review later this week but feels like it could be simplified, I can be wrong.

This is a really good point. For instance, why do we need a whole new function called TryCombineInsertedNaKrulNote() when we already have TryCombineNaKrulNotes()? The functionality has to be similar between these two functions. Can they not be combined in some way?

I think the two call sites represent different semantics: TryCombineNaKrulNotes() handles the ground-pickup/world-item path, while TryCombineInsertedNaKrulNote() handles an item already inserted into InvList, where removals can compact inventory and shift indices. So I do think a separate entry point is justified.
That said, I agree there is shared combine logic here, and I can refactor it so both paths delegate to a smaller common helper while keeping the separate wrappers for their different contexts.

Do I have green light?

[StephenCWills]

Yes, that's what I expected you might say.

[qndel]

what about a scenario where someone would want to keep multiple torn note fragments in stash? after this "fix" it will be impossible to keep all 3, I think having them merge only when picking up from the ground is enough

[morfidon]

what about a scenario where someone would want to keep multiple torn note fragments in stash? after this "fix" it will be impossible to keep all 3, I think having them merge only when picking up from the ground is enough

not unless we want to consciously design stash as a way to bypass auto-combine? If not, combining in inventory-insert flow is more consistent than limiting it to ground pickup only.

"merge only on ground pickup" is, to me, semantically weaker than "merge on relevant player inventory insert flows." Because then the result depends on the random path to acquiring the item, not on the game rule itself.

[morfidon]

Yes, that's what I expected you might say.

I extracted the shared "other two notes" rule into a helper while keeping the two entry points separate because they operate on different state. If you think it would still be better to unify this further, I can take another pass
commit b2ccedf

[Ruler2112]

I'm not familiar enough with github to know how to download the appimage snapshot for this change - sorry. 🙁 I'm attaching a save game for you - I slogged through the crypt and managed to get all 3 torn notes. They're in my stash.
RulerNotes.zip

[morfidon]

I'm not familiar enough with github to know how to download the appimage snapshot for this change - sorry. 🙁 I'm attaching a save game for you - I slogged through the crypt and managed to get all 3 torn notes. They're in my stash. RulerNotes.zip

That's totally ok! Thank you man it helped a lot.

I can confim the fix is working properly. When I move notes from stash to inventory they are being combined now.

They do not combine when I move last note TO the storage.

Should we change it or keep it as is.