-
Notifications
You must be signed in to change notification settings - Fork 68
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
Destat NPC #119
base: main
Are you sure you want to change the base?
Destat NPC #119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, thanks for this PR.
I've left a few comments and questions. I've read the code but I haven't actually tested it.
It'll be up to @kaansoral to decide whether to merge or not but it mostly looks good to me.
node/server.js
Outdated
|
||
socket.on("destat", function (data) { | ||
var player = players[socket.id]; | ||
var item = player.items[data.item_num]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable was num
in the client-side function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
var item = player.items[data.num]; | ||
if (!item) { | ||
return fail_response("no_item"); | ||
} | ||
var def = G.items[item.name]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you redefine item
and def
? Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied a lot of the code from socket.on("locksmith")
, it seems there is duplicate code there too.
if (!player || player.user) { | ||
return fail_response("cant_in_bank"); | ||
} | ||
if (!player.computer && simple_distance(G.maps.desertland.ref.scrollsmith, player) > B.sell_dist) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probable use distance
for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simple_distance
is consistent with locksmith, which I based the code on.
var scrolltype = item.stat_type + "scroll"; | ||
if (scrolltype == "mp_costscroll") { | ||
scrolltype = "mpcostscroll"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try with all scrolls to make sure they all follow the pattern except mpcostscroll
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
Remove duplicate item definitions.
This pull request adds an NPC to Desertland that allows players to remove the stat modifier from an item that has had one applied via scrolls. This process only works on items that have a stat applied to them, i.e.
item.stat_type != null
.The number of scrolls returned is equivalent to the number of scrolls that would be required to apply the stat to the item if it were at level 0. For example, if Wanderer's Breeches (common at +0) were upgraded to level 7 (they are High grade at +7), and then Intelligence scrolls were used to apply Intelligence to the item (10 scrolls would be required), and then the item were brought to this NPC, the NPC would return only 1 Intelligence scroll.
Additionally, the cost scales with the number of scrolls that would be returned - it is 10 times the value of
G.items[scrollname].g
times the quantity of scrolls returned. For our example with Wanderer's Breeches +7, it would cost 8,000 gold (at the time of writing, prices may change for rarer scrolls).As a precaution, the NPC cannot return scrolls for which there exist no scroll definition: Suppose that an item somehow had "charisma", then the NPC would not return an invalid "charismascroll" item.
The NPC can be found in Desertland, near the purple Scorpion spawn.