Implement Handler For Opcode 8138 #490
Conversation
| { | ||
| for(auto item: *container->inventory()) | ||
| { | ||
| if(item->PID() == 519) |
There was a problem hiding this comment.
Why is this number hardcoded and what it stands for?
There was a problem hiding this comment.
that's bottlecap Prototype ID according to the fallout wiki. I wasn't sure how to check what item it was. It's definitely not good because fallout 1 bottlecap prototype id is different.
Maybe change it to check by item name, or is there some better way to identify items?
There was a problem hiding this comment.
It's bad practice to hard-code magic numbers. Replace with ENUM reference (as temporary solution), or implement properly (like checking if it's an actual container).
Best way to handle such cases is to look how it's implemented in original engine.
There was a problem hiding this comment.
the only other ways i can think of doing it is an enum full of all the item pid's and reference it from this enum, but bottlecaps pid would still be hardcoded, just in an enum
The other way would be searching through the inventory vector of items by name, which would probably be pretty slow if it needs to do that often and the item name would be hardcoded.
How would i check how the original engine did it?
There was a problem hiding this comment.
You can't check by name, because name is loaded from translation files, it's different depending on game installation...
I just checked the engine, you just iterate over inventory contents and compare PID with enum value of PID_BOTTLE_CAPS and also use recursion if item subtype is container.
There was a problem hiding this comment.
Should it be PID_BOTTLE_CAPS or PID_REAL_BOTTLE_CAPS? Also, /src/Format/Enum.h should be where this enum is defined, right?
There was a problem hiding this comment.
In Fallout 2 caps PID is 41. It is hard-coded in vanilla engine in many places, but I strongly believe that we shouldn't hard-code any PID's... But for now I think you can just use enum. We will replace it later when we implement scripting and extended configs/prototypes.
bludwulf
changed the title
| unsigned int Opcode8138::countBottlecaps(std::vector<Game::ItemObject*>* inventory, unsigned int bottlecaps) | ||
| { | ||
| unsigned int totalBottlecaps = bottlecaps; | ||
| for(auto item: *inventory) |
| if(auto containerItem = dynamic_cast<Game::ContainerItemObject*>(item)) | ||
| totalBottlecaps += countBottlecaps(containerItem->inventory(), totalBottlecaps); | ||
| else if(item->PID() == (int)ITEM_PID::PID_BOTTLE_CAPS) | ||
| totalBottlecaps += item->amount(); |
There was a problem hiding this comment.
Put conditional blocks in brackets.
src/VM/Handler/Opcode8138Handler.cpp
Outdated
|
|
||
| // Falltergeist includes | ||
| #include "../../Logger.h" | ||
| #include "../../VM/Script.h" | ||
| #include "../../Game/ContainerItemObject.h" | ||
| #include "../../Game/CritterObject.h" | ||
| #include "../../Game/ItemObject.h" | ||
| #include "../../Format/Enums.h" |
There was a problem hiding this comment.
Not sure if it's appropriate to use File Format-related enums in our engine code. Basically everything under Format namespace should be used solely when loading files (via ResourceManager).
src/VM/Handler/Opcode8138Handler.cpp
Outdated
| else | ||
| { | ||
| _error("item_caps_total: object type has no inventory!"); | ||
| _script->dataStack()->push(0); | ||
| } | ||
| } | ||
|
|
||
| unsigned int Opcode8138::countBottlecaps(std::vector<Game::ItemObject*>* inventory, unsigned int bottlecaps) |
There was a problem hiding this comment.
I think the intention of this code will be clearer if you use reference instead of a pointer for inventory argument. Here it's not clear if inventory can be null or not.
Also remove the bottlecaps argument. It's useless.
Handler for opcode 8138 mostly done, but it doesn't take into account container recursion yet. (like if you have a bag in your inventory with bottlecaps inside of it).