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
900 stars 758 forks source link

Missing several features for Random Option #2379

Open AnnieRuru opened 5 years ago

AnnieRuru commented 5 years ago

A full list of (Missing) Random Options

btw I don't really have time to do this ... hopefully some kind souls can help out

Cyanide0210 commented 5 years ago

Wouldn't be better to add params to the "2" variation of the command? for random options instead of creating another ones?

AnnieRuru commented 5 years ago

getitem2/delitem2/countitem2/makeitem2/rentitem2/getitembound2 are already exist in hercules

by the way if you don't know what upstream merge means, means all these stuffs already implement in rathena ... also means ... we can just steal their code

Emistry commented 5 years ago

btw, why prefer to have new getitem3 delitem3 and etc? why not just consider the existing getitem2 delitem2 and etc as option? just make those new fields as optional field?

AnnieRuru commented 5 years ago

because the account ID parameter gets in the way

*getitem(<item id>, <amount>{, <account ID>})
*getitem2(<item id>, <amount>, <identify>, <refine>, <attribute>, <card1>, <card2>, <card3>, <card4>{, <account ID>})

rathena getitem3

*getitem3 <item id>,<amount>,<identify>,<refine>,<attribute>,<card1>,<card2>,<card3>,<card4>,<RandomIDArray>,<RandomValueArray>,<RandomParamArray>{,<account ID>};

this is exactly the reason why Haru don't really like adding stuffs like

*setmount({<flag>{,<account ID>}})

if somehow later we need to expand it, the account ID gets in the way of expansion https://github.com/HerculesWS/Hercules/pull/2082#issuecomment-397606770

dastgirp commented 5 years ago

I have no idea where my response has gone (maybe bad internet), so writing it again: I have a suggestion, Maybe just have getitem command with item_id and amount which returns inventory idx (or indexes). and then have other commands setitemoptions? to set identify,refine, and other parameters, so that this command could be extended without having the trouble of account_id and without needing new commands everytime.

Or maybe merge them into one with variable parameters: like 1) getitem(A,B) -> item, amt 2) getitem(A,B,C) -> Case 1 with account_id(C) 3) getitem(A,B,C,D,E,F,G,H,I) -> getitem2 4) Case 3 with one more variable -> last will be recognized as account id and so on.

AnnieRuru commented 5 years ago

Maybe just have getitem command with item_id and amount which returns inventory idx (or indexes). and then have other commands setitemoptions?

we did tried to make getinventorylistidx a few days back, and we found out the inventory index could be empty if item was deleted ... I think its unsafe to use in scripting now, it probably missing VECTOR_ERASE or ARR_MOVELEFT

Or maybe merge them into one with variable parameters: like

also thought of that before, but this is probably unsafe to use ... because doing check like you mentioned, if the scripter made mistakes on the amount paramater ...

having only 1 script command that accepts all just like your suggestion will be 

    BUILDIN_DEF(getitem,"vi*"),

that only throw error during the execution time

but having multiple ones ...

    BUILDIN_DEF(getitem,"vi?"),
    BUILDIN_DEF2("getitem2",getitem,"viiiiiiii?"),
    BUILDIN_DEF2("getitem3",getitem,"viiiiiiiilll?"),

can throw error during parse time

dastgirp commented 5 years ago

we did tried to make getinventorylistidx a few days back,

Why do you need new command for getting idx of all the items? getinventorylist is enough. @inventorylist_idx variable can be introduced to it if we need all indexes. The similar implementation can be copied to getitem to return the indexes.

I agree the second suggestion is not good, since it would not be able to detect errors while parsing.

AnnieRuru commented 5 years ago

because *getinventorylist script command ignore the missing index in your inventory just try my *getinventorylistidx script command

Zarbony commented 4 years ago

up