Keep track of all mobs that drop an item #8049
Conversation
Don't reload the entire db when adding or deleting drops Fixes rathena#7845
| int dropchance = id->mob[j].chance; | ||
| std::shared_ptr<s_mob_db> mob = mob_db.find(id->mob[j].id); | ||
| if(!mob) continue; | ||
| // NB: We don't const-ref this because we modify dropchance, so let's just make a copy |
There was a problem hiding this comment.
Wouldnt it be more performant to const-ref this and just have a separate variable for the "real" droprate calculation?
There was a problem hiding this comment.
ehh, two 32bit copy or a 64bit address with a 32bit copy, not sure which is better
src/map/atcommand.cpp
Outdated
| } else { | ||
| sprintf(atcmd_output, msg_txt(sd,1287), MAX_SEARCH); // - Common mobs with highest drop chance (only max %d are listed): | ||
| clif_displaymessage(fd, atcmd_output); | ||
| return 0; |
There was a problem hiding this comment.
| return 0; | |
| continue; |
It can be more than one item, so sadly we cannot cancel early.
src/map/script.cpp
Outdated
| if (!mob) { | ||
| if (script_isstring(st, 2)) | ||
| ShowWarning("delmonsterdrop: bad mob name given %s\n", script_getstr(st, 2)); | ||
| else | ||
| ShowWarning("delmonsterdrop: bad mob id given %d\n", script_getnum(st, 2)); | ||
| return SCRIPT_CMD_FAILURE; | ||
| } |
There was a problem hiding this comment.
Can we combine that with the if/else right at the start?
The item db lookup is kind of useless, if we do not have a valid mob.
Also please change it to ShowError
src/map/script.cpp
Outdated
| return SCRIPT_CMD_FAILURE; | ||
| } | ||
|
|
||
| unsigned char i; |
There was a problem hiding this comment.
Please change this to be the loop variable.
| script_pushint(st,1); | ||
| return SCRIPT_CMD_SUCCESS; |
There was a problem hiding this comment.
I think this was broken sometime in the past, since we support monsters dropping the same item id multiple times now.
src/map/script.cpp
Outdated
| mob->dropitem[i].steal_protected = false; | ||
| mob->dropitem[i].randomopt_group = 0; | ||
|
|
||
| std::shared_ptr<item_data> itm = item_db.find(item_id); |
There was a problem hiding this comment.
There is already the following call at the start:
if(!item_db.exists(item_id)){
Please rewrite it and only look it up once at the start of the function.
src/map/script.cpp
Outdated
| auto it = std::find_if(itm->mobs.begin(), itm->mobs.end(), [&mob](const auto &drop) { | ||
| return drop.mob_id == mob->id; | ||
| }); | ||
| if (it != itm->mobs.end()) | ||
| itm->mobs.erase(it); |
There was a problem hiding this comment.
Why dont we std::remove_if here?
There was a problem hiding this comment.
we only want to remove one drop, remove_if moves all to the end, so i went with find
src/map/mob.cpp
Outdated
| id->mobs.end(), | ||
| drop, | ||
| [](const auto &drop, const auto &it) { | ||
| return drop.chance < it.chance; |
There was a problem hiding this comment.
Why is this the only place where we use <?
All others use >.
| int chance; | ||
| int id; | ||
| } mob[MAX_SEARCH]; //Holds the mobs that have the highest drop rate for this item. [Skotlex] | ||
| struct drop_chance { |
There was a problem hiding this comment.
Would be nice to move this outside and follow the naming standards.
There was a problem hiding this comment.
What's the naming standards?
This struct only has a meaning inside item_data, so i made it a struct inside.
src/map/itemdb.hpp
Outdated
| uint32 chance; | ||
| uint32 mob_id; | ||
| }; | ||
| std::vector<drop_chance> mobs; // Sorted vector that holds the mobs that have the highest drop rate for this item. |
There was a problem hiding this comment.
This vector has no limit now anymore.
Lemongrass3110
added
component:core
@whodropsnot work correctly withaddmonsterdrop#7845Don't reload the entire db when adding or deleting drops
Fixes #7845