search

HerculesWS / Hercules

Hercules is a collaborative software development project revolving around the creation of a robust massively multiplayer online role playing game (MMORPG) server package. Written in C, the program is very versatile and provides NPCs, warps and modifications. The project is jointly managed by a group of volunteers located around the world as well as a tremendous community providing QA and support. Hercules is a continuation of the original Athena project.
http://herc.ws
GNU General Public License v3.0
903 stars 757 forks source link

`getmobdrops` refactor #3319

Closed jasonch35 closed 1 month ago

jasonch35 commented 2 months ago

Pull Request Prelude

Changes Proposed

Avoid use of global variables that tend to get overwritten when command is called by 2 or more players simultaneously. The command now requires an array as the second or third argument. The third argument is optional and, if provided, will copy the drop rates into the array. Updated documentation to reflect these changes.

Usage:

.@count = getmobdrops(.@mob_id, .@item, .@rate);
if (.@count) {
    mes(getmonsterinfo(.@mob_id, MOB_NAME) + " - " + .@count + " drops found:");
    for (.@i = 0; .@i < .@count; ++.@i) {
        mes(.@item[.@i] + " (" + getitemname(.@item[.@i]) + ") " + .@rate[.@i]/100 + ((.@rate[.@i]%100 < 10) ? ".0":".") + .@rate[.@i]%100 + "%");
    }
} else {
    mes("No drops found.");
}
close();

Issues addressed: Wrong data when multiple players call the command.

skyleo commented 2 months ago

May want to consider to deprecate the command instead, to not screw over existing scripts too much. And instead use a new name getmonsterdrops, so that when the deprecated command is used, a warning is shown on map-server saying to use getmonsterdrops instead.

Alternatively one can go the hard-route but you really have to ensure and be aware of what is shown to people using older scripts then, it has to not silently fail 100%. And it definitely would be good if it fails at parsing already and not at runtime, otherwise people may start the server and think all is fine after the update containing this change.

Ideally this PR should already replace all existing scripts with the new command.

Good work though.

jasonch35 commented 2 months ago

@skyleo Good point, I hadn’t considered that. However, both getpartymember and getguildmember underwent similar upgrades and were merged without deprecation. 😄 Should I still create a new command and deprecate getmobdrops?

skyleo commented 2 months ago

@skyleo Good point, I hadn’t considered that. However, both getpartymember and getguildmember underwent similar upgrades and were merged without deprecation. 😄 Should I still create a new command and deprecate getmobdrops?

Although I don't fully remember the behavior, the script engine may fail older scripts on parsing already due to the mandatory parameter amount being different between the older and your version of the script command.

So I guess you may be fine. May be worth testing though. ;)

MishimaHaruna commented 1 month ago

Although I don't fully remember the behavior, the script engine may fail older scripts on parsing already due to the mandatory parameter amount being different between the older and your version of the script command.

That's right, since the old command had only one argument and the new one has two mandatory ones and an optional one, it'll already error out when trying to load an old script, forcing the user to notice and update the script. There's no need to deprecate and renamed the command in this case