Missing several features for Random Option

[AnnieRuru]

A full list of (Missing) Random Options

• [x] Initial commit for Item Random Option (https://github.com/HerculesWS/Hercules/pull/1598)
• [ ] getitem3/delitem3/countitem3
• [ ] makeitem3
• [ ] rentitem3
• [ ] getitembound3
• [x] Support monster drops with Random Options (https://github.com/HerculesWS/Hercules/pull/2309)
• [x] getinventorylist/getcartinventorylist <-- I think script.c already done, just not yet update documentation
• [ ] rodex_sendmail3
• [x] @sold_option_id/@sold_option_val/@sold_option_param from OnSellItem: label, *callshop script command

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

[Cyanide0210]

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

[AnnieRuru]

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]

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]

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]

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]

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]

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]

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

[Zarbony]

up