fix #313 Eliza Quinn interaction 'Milking' does not re-equip bra afterwards

[Zsar]

Implemented a quick-and-dirty fix on just the quick milking scene.

Found that fixing this properly would touch quite a lot of code, so I am asking for directions first.

... Seeing that one would have to retrofit any scene-by-scene with this capability makes me feel a bit nauseous. Even with a dedicated helper class, it would still mean two calls and one variable per scene, which may always be forgotten or otherwise go out of sync between code changes.

Maybe SceneBase could get its own inventory handling functions and encapsulate the helper class, such that individual scenes only need to decide whether items should be replaced or not? That way only one call (per item) can be forgotten, the one that stores said item in the first place.

[Zsar]

Should probably become its own class 'ItemReEquipHelper' or similar, encapsulating re-equip behaviour in a single function.

displacedItems: ItemBase[]

func displace(item: ItemBase) -> void
func reEquipAll() -> void

or maybe
func reEquipAll(discardDisplacers: bool) -> void
or even
func reEquipAll(displacersGoHere: Optional<Inventory>) -> void
so one need not remove temporary items like the pumps manually and (in the latter case) may dynamically equip persistent items from an inventory without fear of accidentally destroying them.

[Zsar]

Can be dropped, if you like.

Seemed sensible, but is not in scope of the issue.

[Alexofp]

This could error out if we're waring portal panties or something. The pp is not reachable but there is no chastity cage. I'd promoute it to a variable and check if its not null

[Zsar]

See comment above: This could and probably should be a single function call, lest one forget some item.

[Zsar]

It would be better to change the return value of forceEquipRemoveOther and assorted functions to be a struct

successfully : bool
itemDisplaced: Optional<ItemBased>

but I did not want to make such a big change without asking you first.

(Don't mind the leg work, though, so if you give the go-ahead, I shall.)

... It also seems that GDScript flat out cannot handle null-ables as return types? So it would also need such an Optional class, which it also does not implement itself?
- Damn, one should think that if someone were to create their own programming language, they'd take care to make it better than its predecessors!

[Alexofp]

Do this really need extra new functions? Can you maybe use the other ones that exist already.
instead of doing displacedItems.addItem(GM.pc.getInventory().forceEquipReturnOther(thePump))
you can get the current item in that slot and unequip it manually

var theItem = GM.pc.getInventory().removeItemFromSlot(thePump.getClothingSlot())
if(theItem):
   displacedItems.addItem(theItem)
GM.pc.getInventory().forceEquipStoreOtherUnlessRestraint(thePump)

Or something x3

[Zsar]

I just wrote the extra function to have a working prototype, but I also dislike adding more. That's why I figured it would be more sensible to change the return value of the existing functions. Just did not want to dump 100+ code changes on you without asking first.

Of course, once there is a dedicated object to deal with these code changes, it does not harm to just do it in several lines - those would no longer pollute the Scene code, they would all be hidden behind a tidy one-liner calling on said object.

Candidates are

• LightInventory (which should then probably be renamed, but de-facto is already used exclusively for item transfers and temporary storage)
• PlayerSlaveryStash (which is a "character", I see, but thus could provide full slot information by equipping what was equipped and storing what was not)
• a new class (my thought yesterday, before discovering PlayerSlaveryStash; now I'd rather use one of the existing two)

[Zsar]

See comment below: If a class were implemented, it would deal with this logic instead of this "free-standing" function.

[Zsar]

It is a bit sad, is it not? *hint, hint, nudge, nudge*

[Zsar]

Had a thought overnight: The ("non-soft") Player Slavery system already contains the functionality to store player equipment for arbitrary times and restore it afterwards, via PlayerSlaveryHolder#transferAllItems, #storePlayersItems, #givePlayerItemsBack.

It might be sensible to not reinvent the wheel here, especially seeing that #318 is basically the same issue as this one (just more destructive).

Should I "just" coopt the PlayerSlaveryStash and move transferAllItems et all into SceneBase or a similar central position?

[Alexofp]

Using LightInventory is fine, I don't see a reason for anything more complicated
You can make it easier by 'unequpping items' instead (so they stay in player's inventory) and storing an array of unique IDs of those items and then re-equipping them after the scene end.

[Zsar]

That's what I had tried before using a separate LightInventory, but it did not always work and I could not figure out why: For some reason, it would never re-equip specifically a Chastity Cage.

Debugging loadData, loading the uniqueId worked, but thengetItemByUniqueID always yielded null, whereas a Laced Bra loaded just fine. I took care to try both having several of both items in inventory, as well as just a single one, and in all cases it was the Chastity Cage that failed to be retrieved.