 |
Another case: https://forums.thedarkmod.com/index.php?/topic/21760-the-dark-mod-211-feedback-thread/&do=findComment&comment=483496 "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." |
|
|
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. |
|
|
At the link above, https://forums.thedarkmod.com/index.php?/topic/21760-the-dark-mod-211-feedback-thread/&do=findComment&comment=483496, 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. |
 |
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?) |
|
|
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? |
|
|
stgatilov: Could you please review this code clean up? https://gist.github.com/daftmugi/f5f83c7668b95ba4e93a065f7572d3fc 1. STIM_FROB is fired in only one location. 2. multiloot checks are moved to a bool variable. |
|
|
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. |
|
|
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. |
|
|
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 "". |
|
|
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. |
|
|
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. |
|
|
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? |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
Committed assets rev 16836. Adds "is_immobile_readable" spawnarg, which will be used later in a code update. |
|
|
Committed code clean up as code rev 10482. |
- Anonymous
