View Issue Details

IDProjectCategoryView StatusLast Update
0006270The Dark ModCodingpublic05.12.2023 01:36
Reporterstgatilov Assigned ToDaft Mugi  
Status resolvedResolutionfixed 
Product VersionTDM 2.11 
Target VersionTDM 2.12Fixed in VersionTDM 2.12 
Summary0006270: Multiloot feature breaks game sometimes
Description1) In "The Accountant 2: New in Town":

"Grab your gear" objective will not complete when using auto-frob. Individual clicking on each item completes the objective.

To replicate:
Exit starting room and head inside the room to your far right, next to a window. Pull the "lever" to open the secret door leading to your gear. See that auto-frobbing will not complete the objective, while individual clicking does.

2) In "Cole Hurst 1: Eaton"
Individual clicking leaves player with 3 functional maps, while auto-frobbing might result with only 1 or 2 of them usable. Auto-frobbing will cause player to pick up, but not acquire map items to inventory.

To replicate:
Upon spawning, drop to lower floor and pick up items on your table by individual clicking. See that you should end up with 3 separate maps, all of which become usable by "cycle maps" key.
Now restart and pick up everything by auto-frobbing. Sometimes you are only left with 1 or 2 maps instead of the 3.

If you keep your eye on pickup messages while auto-frobbing, you can briefly notice "acquired -", instead of "acquired (map item name)"
You can also see that once first map is picked up, you might have to release the "frob" key and click again to pick up other maps, while other item types do not need the key to be released.
Also, inventory item in the corner of the screen might say "special" with no visible icon.
Additional InformationOriginally reported:
TagsNo tags attached.


Daft Mugi

Daft Mugi

14.03.2023 05:30

developer   ~0015972

Another case:

"This might have been mentioned before but the auto-frobbing feature should only work for loot. Currently it also works for weapons laying next to each other (for example in map2 of A house of locked secrets), but if you do that then the hud items don't appear when you use them, making them impossible to use."
Daft Mugi

Daft Mugi

04.09.2023 19:47

developer   ~0016057

Committed fix as code rev 10448.

The main bug was caused by the STIM_FROB response not being fired on entities when multilooting.

Fixes include:
* Fire STIM_FROB when multilooting.
* Do not try to multiloot on immobile readables.
* Skip use-on-frob when multilooting.

Also, some code clean up.
Daft Mugi

Daft Mugi

07.09.2023 16:39

developer   ~0016058

At the link above,,
the mission is "A House of Locked Secrets" on map "manor" at viewpos "1462.98 1048.2 -11.75 31.3 103.5 0.0".

Also of note, the immobile readable on the desk would get displayed on screen during multiloot. That seemed unexpected and annoying.
This was addressed in code rev 10448.


19.09.2023 07:41

administrator   ~0016066

Thanks for fixing this!

I have a few comments about the change:

1) Checking for "snd_acquire" is surely a bad approach.
  Does the behavior on single frob depend on the sound spawnarg?
  If not, we'd better find what it depends on, and use the same condition.

2) You added TriggerResponse(ST_FROB) under a new if ((impulseState == ERepeat) && multiloot).
  Why not just add this condition to line 11469?
  If it is possible to trigger frob stim from one code location, better do it.
  Especially since (impulseState == EPressed || ((impulseState == ERepeat) && multiloot)) condition is already present (maybe put it into some bool variable?)
Daft Mugi

Daft Mugi

19.09.2023 16:11

developer   ~0016069

Thanks for doing a code review!

Ok, I'll create a bool variable for ((impulseState == ERepeat) && multiloot) and commit to SVN.

1. Re checking for "snd_acquire". If there's a better solution, I'd be happy to do that. What I originally wanted was a "readable" property or "immobile_readable" property to check for, but there isn't one. Then, I found that items that are lootable all have the "snd_acquire" property in common, so I checked for that. If the player loots and there's no feedback via sound, then don't multiloot it. Perhaps the player wouldn't notice it if there wasn't a sound. That was my thinking. Does that sound ok now? Is there another way?

2. I originally checked for it on line 11469. But after looking at the code more closely, I decided it would be better to fire the STIM_FROB response after the multiloot checks were done and after scripts had a chance to run. I only wanted the stim to fire if multiloot was most likely to happen. Does that sound ok?
Daft Mugi

Daft Mugi

19.09.2023 16:48

developer   ~0016070

stgatilov: Could you please review this code clean up?

1. STIM_FROB is fired in only one location.
2. multiloot checks are moved to a bool variable.


19.09.2023 21:09

administrator   ~0016071

Yeah, the patch looks great!

Well, I just feel it is not a good idea to tie game logic of whether readable can be picked to a symptom of whether it results in a sound.
I think we should investigate why these immobile readables are not put into inventory on ordinary frobs (unlike normal readables), what's the difference there.
Then we'll see the proper condition on whether readable is immobile or not.
(ideally it's great to not duplicate this check in the code, but if the check is trivial it's not important)

If the idea sounds confusing, I can try to look --- maybe I'll be able to find the core criterion.
Daft Mugi

Daft Mugi

19.09.2023 21:30

developer   ~0016072

An immobile readable is meant to be read (displayed) without being put into the player's inventory.
It is the default for readables.

The mobile readable overrides that behavior, putting the readable into the player's inventory without reading it (displaying it on screen) first.

These are defined in the "def/tdm_readable.def" file.

The immobile readable also has the following def properties:
  "frob_action_script" "readable_immobile_frob"
  "scriptobject" "darkmod_readable_immobile"
But I didn't want to check for a script as a mission author might override those.

Multiloot was already checking for "inv_name" property, so to me, checking for "snd_acquire" wasn't much different.
If a mission author overrides "snd_acquire" or forgets to add a "snd_acquire" property, the worst that would happen is that multiloot would not work for that item and the player would have to loot it individually.
But again, if there's a better way to identify an immobile readable, I'd be happy to know.


20.09.2023 18:55

administrator   ~0016075

Last edited: 20.09.2023 18:56

I looked this case.

Well, normally mobile readable is distinguished from immobile by having "inv_name": atdm:readable_mobile has it, atdm:readable_immobile does not.
And this check is already used for that purpose.

The problem is: the mapper has decided to specify "inv_name" for this particular readable.
So this very readable turns out to be a corner case (aka mapper issue), probably not the only one.
And indeed: thinking about it, having "inv_name" does not break immobile readable in any way, nobody forbids to put some unused informational spawnarg, right?

So going back to the difference in behavior: the "scriptobject" spawnarg looks interesting.
atdm:readable_mobile has "darkmod_readable_mobile".
atdm:readable_immobile has "darkmod_readable_immobile".
However, looking at the code, adding entity to inventory happens regardless of what script function does.

I looked inside AddToInventory calls (at Player.cpp:11532).
This very item is not added because it has no/empty "inv_category" spawnarg.
If it had inv_category too, then player would actually take the item into inventory (and perhaps display it at the same time).

Note that there are other code branches there (ValidateLoot, ValidateAmmo, ValidateWeapon).
They are handled before generic items, and it seems that not all of these items require inv_category.

I guess the best solution would be to add method Inventory::CanPutItem near PutItem method.
There make checks for the main inv_XXX spawnarg from ValidateLoot/ValidateAmmo/ValidateWeapon.
If none triggers, then check that both inv_name and inv_category are set.
It takes more code, but feels much better than checking for sound.

And one more issue is that we should check if inv_name + inv_category are either missing OR EMPTY.
Mapper might decide to make an entity unpickable by inheriting its def and overriding e.g. inv_category to "".
Daft Mugi

Daft Mugi

20.09.2023 22:08

developer   ~0016076

What if we add an "immobile_readable" property to "atdm:readable_immobile"?

// def/tdm_readable.def
entityDef atdm:readable_immobile
  "inherit" "atdm:readable_base"
  "immobile_readable" "1"

Then, do multiloot if:
target->spawnArgs.GetInt("immobile_readable", "0") != 1;

I tested this idea. Works well so far.


21.09.2023 06:30

administrator   ~0016077

Better add properly "can_multiloot" then.
Although to me it still sounds like a last-resort solution, because the game can already determine whether item can be picked to inventory or not.
Adding one more condition which should be equivalent to it will make things worse.

It's just unlucky that the exact condition is a bit more complicated than "if spawnarg XXX exists".
So it's not a one-line fix, some refactoring is necessary.
Daft Mugi

Daft Mugi

22.09.2023 22:42

developer   ~0016079

My understanding is as follows:

The main problem with the immobile readable is not whether it can be added to the inventory, because it does not matter if the immobile readable is added to the inventory or not during multiloot. Therefore, `Inventory::CanPutItem()` might not make a difference with some immobile readables, because like you said, "If it had inv_category too, then player would actually take the item into inventory (and perhaps display it at the same time)."

The main problem is that the immobile readable is displayed on the screen during multiloot.
The `target->FrobAction(true)` line calls the `readable_immobile_frob` script, which displays the immobile readable.
That is unrelated to the inventory.

By adding the `immobile_readable` spawnarg to the def file, it is explicitly declared and, therefore, can be detected directly.
Then, the `repeatMultiloot` bool variable assignment can become:
const bool repeatMultiloot = multiloot
  && impulseState == ERepeat
  && target->spawnArgs.GetString("inv_name", nullptr) != nullptr
  && target->spawnArgs.GetInt("immobile_readable", "0") != 1;
With that, multiloot will only frob non-immobile readable items that have an `inv_name` spawnarg.

During multiloot, only checking for the `inv_name` spawnarg is most likely enough to identify items that `AddToInventory()` should attempt to add to the inventory, because that's already better than a regular (non-multiloot) frob, which doesn't do any spawnarg checking before calling `AddToInventory()`.

How does that sound?


23.09.2023 10:57

administrator   ~0016081

Ok, if we only want to avoid some modal GUI popping up, then "readable_immobile_frob" is the only problem.
Yes, adding a new spawnarg is OK then.
Maybe better add something like "ignore_multiloot" and set it instead.


18.10.2023 16:20

administrator   ~0016131

So, what do we do with "snd_acquire"?

As far as I understand, we add some spawnarg to readable.
It's either "ignore_multiloot", which can in principle be reused on other stuff (if necessary), or we add kinda typeinfo identifier "is_immobile_readable" and do the switch in C++ code.
Daft Mugi

Daft Mugi

19.10.2023 17:49

developer   ~0016135

I'll use "is_immobile_readable" as an explicit check and for self-documenting code, since the undesired result is for the immobile readable to show on screen during multiloot.

This code change is blocked until new frob (0006316) is resolved as my patch for this assumes that the new frob patch was applied first. It's sitting in my local repo ready to be applied when the time comes.
Daft Mugi

Daft Mugi

20.10.2023 18:46

developer   ~0016142

Committed assets rev 16836.

Adds "is_immobile_readable" spawnarg, which will be used later in a code update.
Daft Mugi

Daft Mugi

30.10.2023 19:41

developer   ~0016157

Committed code clean up as code rev 10482.

Issue History

Date Modified Username Field Change
12.03.2023 16:11 stgatilov New Issue
14.03.2023 05:30 Daft Mugi Note Added: 0015972
04.09.2023 19:47 Daft Mugi Note Added: 0016057
04.09.2023 19:48 Daft Mugi Assigned To => Daft Mugi
04.09.2023 19:48 Daft Mugi Status new => resolved
04.09.2023 19:48 Daft Mugi Resolution open => fixed
05.09.2023 01:57 nbohr1more Fixed in Version => TDM 2.12
07.09.2023 16:39 Daft Mugi Note Added: 0016058
19.09.2023 07:41 stgatilov Note Added: 0016066
19.09.2023 16:11 Daft Mugi Note Added: 0016069
19.09.2023 16:48 Daft Mugi Note Added: 0016070
19.09.2023 21:09 stgatilov Note Added: 0016071
19.09.2023 21:30 Daft Mugi Note Added: 0016072
20.09.2023 18:55 stgatilov Note Added: 0016075
20.09.2023 18:56 stgatilov Note Edited: 0016075
20.09.2023 22:08 Daft Mugi Note Added: 0016076
21.09.2023 06:30 stgatilov Note Added: 0016077
22.09.2023 22:42 Daft Mugi Note Added: 0016079
23.09.2023 10:57 stgatilov Note Added: 0016081
28.09.2023 19:38 stgatilov Status resolved => assigned
18.10.2023 16:20 stgatilov Note Added: 0016131
19.10.2023 17:49 Daft Mugi Note Added: 0016135
20.10.2023 18:46 Daft Mugi Note Added: 0016142
30.10.2023 19:41 Daft Mugi Note Added: 0016157
30.10.2023 19:42 Daft Mugi Status assigned => feedback
05.12.2023 01:36 nbohr1more Status feedback => resolved