Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getmobdrops refactor #3319

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jasonch35
Copy link
Contributor

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.

Avoid the use of global temporary variables
@skyleo
Copy link
Contributor

skyleo commented Sep 17, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

skyleo commented Sep 21, 2024

@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. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants