Implementation of Item Options System. #1598

sagunkho · 2017-03-02T11:30:11Z

Allows the infusing of equipments with up to 5 bonus item options.
Discussion Topic - http://herc.ws/board/topic/14021-random-item-option-system/

This feature is constrained to clients of packet versions greater than or equal to 20150226.
Item Options and their effects are defined server-side in db/item_options.conf and client side in data/luafiles514/lua files/datainfo/addrandomoptionnametable.lub
The ID of the option must tally with the correct index of the description provided in the client side lua file.
A basic custom script to npc/custom/item_options.txt to make use of this feature and demonstrate examples of usage.
IT_OPT_* keys and MAX_ITEM_OPTIONS macro are also exported from the source as constants.
An additional flag disable_options has been added to sql, and as DisableOptions: true/false (boolean, defaults to false !!for equipments only!!) to db/<pre-re/re>/item_db.conf files.
Script commands documentation is included.
SQL File updates are included.

Credits: smokexyz/Hercules
Code Style Formatting by Asheraf/Hercules
Initial design idea: secretdataz/rAthena

This change is

dastgirp · 2017-03-02T12:38:47Z

db/item_options.conf

+//= This file is part of Hercules.
+//= http://herc.ws - http://github.com/HerculesWS/Hercules
+//=
+//= Copyright (C) 2014-2016  Hercules Dev Team


dastgirp · 2017-03-02T12:42:15Z

npc/custom/item_options.txt

+	mes(.@name$);
+	mes("See you around!");
+	close;
+}


Newline missing

dastgirp · 2017-03-02T12:47:29Z

src/map/clif.c

+				j++;
+			}
+		}
+		for (; j < MAX_ITEM_OPTIONS || j < 5; j++) {


MAX_ITEM_OPTIONS is there, why < 5 check?

This is in the case that MAX_ITEM_OPTIONS is < 5. The remaining buffer is appended with 0. If MAX_ITEM_OPTIONS > 5, it is made to revert to 5 in mmo.h.

secretdataz · 2017-03-02T15:05:04Z

:)

Jedzkie · 2017-03-02T15:40:31Z

db/item_options.conf

+{
+	Id: 1
+	Name: "VAR_MAXHPAMOUNT"
+	Script: <" bonus bMaxHP,getequippedoptioninfo(IT_OPT_VALUE); ">


Space between comma

bonus bMaxHP, getequippedoptioninfo(IT_OPT_VALUE);

Jedzkie · 2017-03-02T15:40:45Z

db/item_options.conf

+{
+	Id: 2
+	Name: "VAR_MAXSPAMOUNT"
+	Script: <" bonus bMaxSP,getequippedoptioninfo(IT_OPT_VALUE); ">


Same as above.

Jedzkie · 2017-03-02T15:41:01Z

db/item_options.conf

+{
+	Id: 3
+	Name: "VAR_STRAMOUNT"
+	Script: <" bonus bStr,getequippedoptioninfo(IT_OPT_VALUE); ">


Jedzkie · 2017-03-02T15:41:07Z

db/item_options.conf

+{
+	Id: 4
+	Name: "VAR_AGIAMOUNT"
+	Script: <" bonus bAgi,getequippedoptioninfo(IT_OPT_VALUE); ">


Same until the last Id.

4144 · 2017-03-02T15:30:48Z

src/common/mmo.h

@@ -256,6 +256,12 @@
 #define MAX_ELESKILLTREE 3
 #endif

+#define MAX_ITEM_OPTIONS 5 // Maximum item options [Smokexyz]


Please add #ifndef MAX_ITEM_OPTIONS before 259 line
and #endif after 263 line
This will allow override this settings from build script without changing code.

4144 · 2017-03-02T15:32:47Z

src/map/clif.c

@@ -2386,14 +2386,26 @@ void clif_addcards2(unsigned short *cards, struct item* item) {
 * @param buf[in,out] The buffer to write to. The pointer must be valid and initialized.
 * @param item[in]    The source item.
 */
-void clif_add_random_options(unsigned char* buf, struct item* item)
+void clif_add_random_options(struct RndOptions *buf, const struct item it)


Why not use pointer here?
const struct item it -> const struct item *it
And why rename it from item to it?

4144 · 2017-03-02T15:34:37Z

src/map/clif.c

-			p.option_data[i].index = 0;
-			p.option_data[i].value = 0;
-			p.option_data[i].param = 0;
+		for (i = 0; i < MAX_ITEM_OPTIONS; i++) {


Block with adding options to pointer probably better move to separate function.
This will allow for plugins to change options what will be send to client

4144 · 2017-03-02T15:36:13Z

src/map/clif.c

-		p->option_data[j].index = 0;
-		p->option_data[j].value = 0;
-		p->option_data[j].param = 0;
+	for (i=0, j=0; i < MAX_ITEM_OPTIONS; i++) {


Can be moved to same function like previous options block from clif_additem

4144 · 2017-03-02T15:37:02Z

src/map/itemdb.c

@@ -1299,6 +1309,69 @@ void itemdb_read_packages(void) {
 	ShowStatus("Done reading '"CL_WHITE"%d"CL_RESET"' entries in '"CL_WHITE"%s"CL_RESET"'.\n", count, config_filename);
 }

+void itemdb_read_options(void) {


Here { should be on new line

4144 · 2017-03-02T15:40:16Z

src/map/script.c

+	int i = -1, index = script_getnum(st,2);
+	struct map_session_data *sd = script->rid2sd(st);
+
+	if (sd == NULL)


Probably here need add script_pushint(st, -1) or like this for prevent script from termination

4144 · 2017-03-02T15:44:52Z

src/map/script.c

+	} else if (equip_index > 0 && equip_index <= ARRAYLENGTH(script->equip))
+		i = pc->checkequip(sd,script->equip[equip_index-1]);
+
+	if (&sd->status.inventory[i] != NULL) {


This condition always true. May be you want check something else?

4144 · 2017-03-02T15:47:16Z

src/map/script.c

+		return false;
+	} else if (equip_index > 0 && equip_index <= ARRAYLENGTH(script->equip))
+		i = pc->checkequip(sd,script->equip[equip_index-1]);
+


Here "i" can have -1, and in next lines it will be buffer underflow
like sd->status.inventory[-1]. This is error

Thank you, checked for in next line as if ( i >= 0 && ...)

FlippAcademy · 2017-03-02T16:03:32Z

Nice 👍

sagunkho · 2017-03-02T16:06:42Z

src/map/clif.c, line 2389 at r1 (raw file):

Previously, 4144 (Andrei Karas) wrote…

Why not use pointer here?
const struct item it -> const struct item *it
And why rename it from item to it?

Why should there be a pointer here when the function isn't writing any data to the pointer?
and because struct item item is is like like weird weird.. 😄 😄

Comments from Reviewable

4144 · 2017-03-02T16:32:27Z

@Smokexyz because passing structs by value is bad idea.
It copy whole struct to stack. Pointers created for avoid this issues.
You can add const to pointers too. Even in two places.

Example: const struct item *const it

First const here mean data in struct item is constant.
Second const mean variable it is constant. Mean you cant assign it new value.

sagunkho · 2017-03-02T19:42:13Z

Review status: 0 of 29 files reviewed at latest revision, 15 unresolved discussions.

db/item_options.conf, line 12 at r1 (raw file):

Previously, dastgir (Dastgir) wrote…

2017

Done.

db/item_options.conf, line 50 at r1 (raw file):

Previously, Jedzkie (Jedzkie) wrote…

Space between comma

bonus bMaxHP, getequippedoptioninfo(IT_OPT_VALUE);

Done.

db/item_options.conf, line 55 at r1 (raw file):

Previously, Jedzkie (Jedzkie) wrote…

Same as above.

Done.

db/item_options.conf, line 60 at r1 (raw file):

Previously, Jedzkie (Jedzkie) wrote…

Same.

Done.

db/item_options.conf, line 65 at r1 (raw file):

Previously, Jedzkie (Jedzkie) wrote…

Same until the last Id.

Done.

npc/custom/item_options.txt, line 234 at r1 (raw file):

Previously, dastgir (Dastgir) wrote…

Newline missing

Done.

src/common/mmo.h, line 259 at r1 (raw file):

Previously, 4144 (Andrei Karas) wrote…

Please add #ifndef MAX_ITEM_OPTIONS before 259 line
and #endif after 263 line
This will allow override this settings from build script without changing code.

Done.

src/map/clif.c, line 2389 at r1 (raw file):

Previously, Smokexyz (smokexyz) wrote…

Why should there be a pointer here when the function isn't writing any data to the pointer?
and because struct item item is is like like weird weird.. 😄 😄

Done.

src/map/clif.c, line 2460 at r1 (raw file):

Previously, 4144 (Andrei Karas) wrote…

Block with adding options to pointer probably better move to separate function.
This will allow for plugins to change options what will be send to client

Done.

src/map/clif.c, line 2468 at r1 (raw file):

Previously, Smokexyz (smokexyz) wrote…

This is in the case that MAX_ITEM_OPTIONS is < 5. The remaining buffer is appended with 0. If MAX_ITEM_OPTIONS > 5, it is made to revert to 5 in mmo.h.

Done.

src/map/clif.c, line 2596 at r1 (raw file):

Previously, 4144 (Andrei Karas) wrote…

Can be moved to same function like previous options block from clif_additem

Done.

src/map/itemdb.c, line 1312 at r1 (raw file):

Previously, 4144 (Andrei Karas) wrote…

Here { should be on new line

Done.

src/map/script.c, line 9034 at r1 (raw file):

Previously, 4144 (Andrei Karas) wrote…

Probably here need add script_pushint(st, -1) or like this for prevent script from termination

Done.

src/map/script.c, line 13704 at r1 (raw file):

Previously, 4144 (Andrei Karas) wrote…

Here "i" can have -1, and in next lines it will be buffer underflow
like sd->status.inventory[-1]. This is error

Done.

src/map/script.c, line 13705 at r1 (raw file):

Previously, 4144 (Andrei Karas) wrote…

This condition always true. May be you want check something else?

Done.

Comments from Reviewable

sagunkho · 2017-03-02T19:42:36Z

Reviewed 29 of 29 files at r1.
Review status: 18 of 29 files reviewed at latest revision, 15 unresolved discussions.

Comments from Reviewable

Asheraf · 2017-03-02T20:36:13Z

db/item_options.conf

+//= This file is part of Hercules.
+//= http://herc.ws - http://github.com/HerculesWS/Hercules
+//=
+//= Copyright (C) 2014-2017  Hercules Dev Team


Asheraf · 2017-03-02T20:36:30Z

db/item_options.conf

+//=========================================================================
+//= Items Options Database
+//============================= Credits ===================================
+// Smokexyz


move this up

Asheraf · 2017-03-02T20:37:55Z

npc/custom/item_options.txt

+//================= Current Version =======================================
+//= 1.0
+//==================== Credits ============================================
+//= Smokexyz


credit already added up

woops, thanks.

Asheraf · 2017-03-02T20:38:29Z

npc/custom/item_options.txt

+	.enable_random_bonus = false;
+
+	/* Item Option Descriptions */
+	setarray .options$[0],"Max HP", "Max SP", "STR", "AGI", "VIT", "INT", "DEX", "LUK";


missed a space .options$[0], "Max HP",

Asheraf · 2017-03-02T20:38:46Z

npc/custom/item_options.txt

+		close;
+	}
+	// Build the Menu.
+	setarray .@position$[1],"Head","Body","Left Hand","Right Hand","Robe","Shoes","Accessory 1","Accessory 2","Head 2","Head 3";


spaces here

personally, its not really a good practice to initialize the array at index 1 ... the index 0 is wasted ...
perhaps you should consider to adjust it to index 0 ?

Copied from the hd refiner file. Although, this is done by taking into consideration the index that is passed to the following getequipisequiped(), getequipisenableopt(), getequipisidentify() checks. It is, in source, checked for if (num > 0 && num <= ARRAYLENGTH(script->equip)). It would fail if the index is 0. And if we began the array at [0] we'd have to add a +1 to all the following places we require the check. @MishimaHaruna @4144 Shouldn't this be changed in source? maybe in a separate commit changing all scripts using such commands.

nope, i mean this ...
setarray .@position$[1]....
it should be
setarray .@position$[0]....
same goes for the other script... hd refiner..in case this is copied from there.

should avoid initialize the array and left the value remain unused.

the index that is passed to the following getequipisequiped(), getequipisenableopt(), getequipisidentify() checks. It is, in source, checked for if (num > 0 && num <= ARRAYLENGTH(script->equip)). It would fail if the index is 0.

// Build the Menu. setarray .@position$,"Head","Body","Left Hand","Right Hand","Robe","Shoes","Accessory 1","Accessory 2","Head 2","Head 3"; for (.@i = 0; .@i <= 10; .@i++) { .@menu$ += (getequipisequiped(.@i + 1) ? getequipname(.@i + 1) : .@position$[.@i] + "-[Not equipped]") + ":"; } .@equip_index = select(.@menu$);

or better

// Build the Menu. setarray .@eqi_list,EQI_HEAD_TOP,EQI_ARMOR,EQI_HAND_L,EQI_HAND_R,EQI_GARMENT,EQI_SHOES,EQI_ACC_L,EQI_ACC_R,EQI_HEAD_MID,EQI_HEAD_LOW; setarray .@position$,"Head","Body","Left Hand","Right Hand","Robe","Shoes","Accessory 1","Accessory 2","Head 2","Head 3"; for (.@i = 0; .@i <= 10; .@i++) { .@menu$ += (getequipisequiped(.@eqi_list[.@i]) ? getequipname(.@eqi_list[.@i]) : .@position$[.@i] + "-[Not equipped]") + ":"; } .@equip_index = .@eqi_list[select(.@menu$) - 1];

I believe these approach are better than having the array creating unused index 0.

Your first case would work, but as I said you'd have to increment most .@i values in the loop by 1, which could be rather troublesome. Your second case, although more visually appealing, involves creating a whole new array[10] to make up for one unused index.

Because it's more logical. It's more elegant, it makes more sense ... to have array start with index 0.

I agree, but this scenario involves passing an incremented value to functions that I believe don't handle indexes properly. And so I'm requesting the input of @MishimaHaruna and/or @4144

Asheraf · 2017-03-02T20:39:50Z

npc/custom/item_options.txt

+	// Build the Menu.
+	setarray .@position$[1],"Head","Body","Left Hand","Right Hand","Robe","Shoes","Accessory 1","Accessory 2","Head 2","Head 3";
+	.@menu$ = "";
+	for (.@i = 1; .@i<=10; ++.@i)


.@i <= 10

perhaps the value .@i <= 10 could change to .@i <= getarraysize(.@array) ? since it actually depend on the array values.
and same with above, the .@i should start from 0 since its actually refer to an index of an array.

They array here is not dynamic in size, therefore there is no need to call on a size check.

EQI_HEAD_TOP (1) - Upper head gear EQI_ARMOR (2) - Armor (Where you keep your Jackets and Robes) EQI_HAND_L (3) - What is in your Left hand. EQI_HAND_R (4) - What is in your Right hand. EQI_GARMENT (5) - The garment slot (Mufflers, Hoods, Manteaus) EQI_SHOES (6) - What foot gear the player has on. EQI_ACC_L (7) - Accessory 1. EQI_ACC_R (8) - Accessory 2. EQI_HEAD_MID (9) - Middle Headgear (masks and glasses) EQI_HEAD_LOW (10) - Lower Headgear (beards, some masks)

btw, if you're referring to this list... to use the number 1 ~ 10 ... it's better to replace with the EQI_ constant.
it could help avoid potential script broken if these EQI values changed in future..... just how what happened in rA when they decided to update the values.

I'm don't think we're on the same page, you can hit me up on the IRC to clarify.

I am just suggesting the usage of dynamic size because users are normally confused with this kind of settings if they are not familiar ..
Example:

User: I added a new item to the array .@position$ but it doesn't show up in the menu, why ?

tho its not really something that really need to focus on ...

Asheraf · 2017-03-02T20:40:12Z

npc/custom/item_options.txt

+	setarray .@position$[1],"Head","Body","Left Hand","Right Hand","Robe","Shoes","Accessory 1","Accessory 2","Head 2","Head 3";
+	.@menu$ = "";
+	for (.@i = 1; .@i<=10; ++.@i)
+		.@menu$ += ((getequipisequiped(.@i))?getequipname(.@i):.@position$[.@i]+"-[Not equipped]")+":";


Asheraf · 2017-03-02T20:40:58Z

npc/custom/item_options.txt

+		.@option_variable = select(.@menu$);
+		next;
+		mes(.@name$);
+		mes("You chose ^009900"+.options$[.@option_variable-1]+"^000000!");


Life-saving function, thanks.

Asheraf · 2017-03-02T20:42:11Z

src/char/char.c

@@ -770,8 +773,12 @@ int char_memitemdata_to_sql(const struct item items[], int max, int id, int tabl
 	SQL->StmtBindColumn(stmt, 9, SQLDT_UINT64,    &item.unique_id,   0, NULL, NULL);
 	for (j = 0; j < MAX_SLOTS; ++j)
 		SQL->StmtBindColumn(stmt, 10+j, SQLDT_SHORT, &item.card[j], 0, NULL, NULL);
+	for (j = 0; j < MAX_ITEM_OPTIONS; ++j) {
+		SQL->StmtBindColumn(stmt, 10+MAX_SLOTS+j*2, SQLDT_INT16, &item.option[j].index, 0, NULL, NULL);


10 + MAX_SLOTS + j * 2

Ridley8819 · 2017-03-02T23:49:43Z

npc/custom/item_options.txt

@@ -0,0 +1,234 @@
+//================= Hercules Script =======================================


Actually not sure about the whole custom script. We talked before to drop support for custom folder entirely. Maybe better to remove it here and offer it in forums?
you also forgot parantheses supposed to be included in all commands, this means also in next(); or close();
exceptions here are break and end

are not this script suppose to be added into the doc/sample/ folder?

@MishimaHaruna regarding the location and need for this file as we had discussed.

Good point about the sample folder, I always forget it exists.

My opinion is:

If the script is meant to be an example for users that want to implement their own custom script, it should go into doc/sample.

If the script is meant to be usable as is, it's fine to keep it in npc/custom - keeping in mind that it'll eventually be split to another repository and won't be maintained officially.

Emistry · 2017-03-03T03:08:11Z

Database

by the way, I think previously Herc was also planning to do this Item Bonus system by storing these item_bonus_id / item_bonus_value in a separate SQL tables, the idea was dropped?

Script Command

do we support script command that allow direct creation of item with item bonus?
like
getitem3( ...................<item_bonus_id1>,<item_bonus_value1>,....,<item_bonus_id5>,<item_bonus_value5>);
or
getitem3( .............. <item_bonus_id_array>,<item_bonus_value_array>);

same goes for these if possible ?

rentitem, makeitem, countitem, delitem ...

about the script command to get the current info of item bonus from selected equipment slot ...
why not just do something like .. getinventorylist ?

getequipoptioninfo( <equipment_slot>{, <option_number>} );
equipment_slot - equipment index
option_number
 - which index of item bonus in the equipment (value 1-MAX_ITEM_OPTIONS lines)
 - 0 = list all index

return an array that consist of info of these options in the equipment.
- @item_option_id[] 
- @item_option_value[]

I am not familiar with the item bonus option, but this script command
*getequipoptioninfo(<equipment_slot>,<type>,<option_slot>);
the type parameter seem like troublesome to use.
Example:
when you try to retrieve the item bonus info from equipments, you gonna have to loop through every single <type> unless you trying to hardcode the parameters value during the usage of this script command

just my opinion...

and by the way is this based on rA implementation? o-o
perhaps should link to their commits.. or credits included for them?

Emistry · 2017-03-03T03:09:58Z

npc/custom/item_options.txt

+//= Smokexyz
+//=========================================================================
+prontera,153,180,4	script	Option Master	4_DOG01,{
+	callsub(StartTalking);


this is unnecessary, remove this line and move the whole StartTalking label part up here .... ?

This was done to add the OnInit configuration at the beginning of the file. I couldn't think of a better way.

OnInit label can be located at any place in the script ... personally, I often place the OnInit label at the end of the script ...

Emistry · 2017-03-03T03:11:12Z

npc/custom/item_options.txt

@@ -0,0 +1,234 @@
+//================= Hercules Script =======================================


are not this script suppose to be added into the doc/sample/ folder?

Emistry · 2017-03-03T03:13:05Z

npc/custom/item_options.txt

+		close;
+	}
+	// Build the Menu.
+	setarray .@position$[1],"Head","Body","Left Hand","Right Hand","Robe","Shoes","Accessory 1","Accessory 2","Head 2","Head 3";


personally, its not really a good practice to initialize the array at index 1 ... the index 0 is wasted ...
perhaps you should consider to adjust it to index 0 ?

Emistry · 2017-03-03T03:14:37Z

npc/custom/item_options.txt

+	// Build the Menu.
+	setarray .@position$[1],"Head","Body","Left Hand","Right Hand","Robe","Shoes","Accessory 1","Accessory 2","Head 2","Head 3";
+	.@menu$ = "";
+	for (.@i = 1; .@i<=10; ++.@i)


perhaps the value .@i <= 10 could change to .@i <= getarraysize(.@array) ? since it actually depend on the array values.
and same with above, the .@i should start from 0 since its actually refer to an index of an array.

Emistry · 2017-03-03T03:17:06Z

npc/custom/item_options.txt

+			mes(.@name$);
+			mes("Please input the bonus amount of ^009900"+.options$[.@option_variable-1]+"^000000 you want to add!");
+			mes("(Min: "+.minimum_bonus_amount+", Max: "+.maximum_bonus_amount+")");
+			input .@value;


consider to use this since there are min and max value.
input(.@value, .minimum_bonus_amount, .maximum_bonus_amount);

Thanks, I wasn't aware.

Emistry · 2017-03-03T03:17:55Z

npc/custom/item_options.txt

+			mes("(Min: "+.minimum_bonus_amount+", Max: "+.maximum_bonus_amount+")");
+			input .@value;
+			next;
+		} while (.@value < .minimum_bonus_amount || .@value == 0 || .@value > .maximum_bonus_amount);


extra condition checking can be avoided if you applied the input(<variable>,<min>,<max);

sagunkho · 2017-03-03T07:09:20Z

@Emistry

by the way, I think previously Herc was also planning to do this Item Bonus system by storing these item_bonus_id / item_bonus_value in a separate SQL tables, the idea was dropped?

Not a very good idea for this implementation.

do we support script command that allow direct creation of item with item bonus?

not yet, no. But something like this is required for crimson weapons?

return an array that consist of info of these options in the equipment.

Can be done, it's a good suggestion.

you gonna have to loop through every single <type>

only IT_OPT_INDEX and IT_OPT_VALUE. (0 or 1)

and by the way is this based on rA implementation? o-o

Well, since the code structure of the methods involved are almost the same between both emulators, there aren't many ways to implement such a system without it looking similar. Consider this a system like item cards, but with added fields. The item_options.conf and script commands allows potential to add custom bonus options (as long as the client side is edited.). The ID field allows specifying the index of the client side equivalent. The param field was dropped since we don't know it's use. There are many improvements in both code and design over rA's implementation. The packet structures were added by Dastgir last year. It is for many reasons not based on theirs.

sagunkho · 2017-03-03T13:06:14Z

2 script commands have been changed to -


*getequipoptioninfo(<equip_index>,<slot>,<type>);

Gets the option information of an equipment.

<equipment_index> For a list of equipment indexes see getequipid().
<slot> can range from 1 to MAX_ITEM_OPTIONS
<type> can be IT_OPT_INDEX (the ID of the option bonus, @see "Id" or "Name" in db/item_options.conf)
       or IT_OPT_VALUE (the value of the bonus script of the equipment, @see "Script" in db_item_options.conf).

returns the value of the slot if exists or -1 for invalid slot, type or slots.


*setequipoption(<equip_index>,<slot>,<opt_index>,<value>);

Set an equipment's option index or value for the specified option slot.

<equipment_index> For a list of equipment indexes see getequipid().
<slot> can range from 1 to MAX_ITEM_OPTIONS
<opt_index> can be IT_OPT_INDEX (the ID of the option bonus, @see "Id" or "Name" in db/item_options.conf)
<value> The value of the type to be set.

returns 0 if value couldn't be set, 1 on success.

db2sql update will be added before a merge.

Atemo · 2017-03-03T14:38:25Z

Well, since the code structure of the methods involved are almost the same between both emulators, there aren't many ways to implement such a system without it looking similar. Consider this a system like item cards, but with added fields. The item_options.conf and script commands allows potential to add custom bonus options (as long as the client side is edited.). The ID field allows specifying the index of the client side equivalent. The param field was dropped since we don't know it's use. There are many improvements in both code and design over rA's implementation. The packet structures were added by Dastgir last year. It is for many reasons not based on theirs.

Logical issue, please review.

sagunkho · 2017-03-03T14:49:17Z

@Atemo what?

4144 · 2017-03-12T21:35:53Z

src/map/itemdb.c

+
+		/* Set Script */
+		t_opt.script = *str ? script->parse(str, filepath, t_opt.index, SCRIPT_IGNORE_EXTERNAL_BRACKETS, NULL) : NULL;
+


Please add here call to function for read additional fields. For example like in mob.c mob->read_db_additional_fields.
This can be usefull for plugins too

MishimaHaruna · 2017-03-16T11:55:35Z

sql-files/main.sql

+  `opt_idx3` SMALLINT(5) UNSIGNED NOT NULL DEFAULT '0',
+  `opt_val3` SMALLINT(5) UNSIGNED NOT NULL DEFAULT '0',
+  `opt_idx4` SMALLINT(5) UNSIGNED NOT NULL DEFAULT '0',
+  `opt_val4` SMALLINT(5) UNSIGNED NOT NULL DEFAULT '0',


Still missing the INSERT INTO sql_updates lines

MishimaHaruna · 2017-03-16T11:57:17Z

src/common/mmo.h

+// Maximum item options [Smokexyz]
+#ifndef MAX_ITEM_OPTIONS
+#define MAX_ITEM_OPTIONS 5
+STATIC_ASSERT(MAX_ITEM_OPTIONS <= 5, "This value is limited by the client and database layout and should only be increased if you know the consequences.");


Can you move this line to after the #endif? (so that if it's defined elsewhere it still asserts)

MishimaHaruna · 2017-03-16T12:02:36Z

src/map/clif.c

-		WBUFW(buf,i*5+0) = 0; // OptIndex
-		WBUFW(buf,i*5+2) = 0; // Value
-		WBUFB(buf,i*5+4) = 0; // Param1
+unsigned int clif_add_random_options(struct ItemOptions *buf, struct item *it)


Yes, it's a bug in the parser. The use of const pointers (just like const scalar variables) was never considered, since it doesn't affect the function's contract with the caller. It should be fixed in the parser, but it's not a very high priority task

MishimaHaruna

Other things to consider that aren't part of the diff:

storage.c: compare_item() claims to check for identical items but isn't considering the item options now.
npc.c: npc_selllist_sub() doesn't handle item options.
buyingstore.c: buyingstore_trade() doesn't handle item options (it currently blocks items that have cards in them, but doesn't handle item options)
I'm not sure how the searchstore system is supposed to handle item options. Did they add new searchstore packets that include these, or should we ignore it for now?

MishimaHaruna · 2017-03-17T10:06:04Z

sql-files/upgrades/2017-03-05--08-09.sql

+	ADD COLUMN `opt_idx4` SMALLINT(5) UNSIGNED NOT NULL DEFAULT '0' AFTER `opt_val3`,
+	ADD COLUMN `opt_val4` SMALLINT(5) UNSIGNED NOT NULL DEFAULT '0' AFTER `opt_idx4`; 
+
+INSERT INTO `sql_updates` (`timestamp`, `ignored`) VALUES (1488744559 , 'No');


Unterminated line at end of file

MishimaHaruna · 2017-03-17T10:13:53Z

src/char/char.c

 				// They are the same item.
 				ARR_FIND(0, MAX_SLOTS, j, items[i].card[j] != item.card[j]);
-				if (j == MAX_SLOTS
+				ARR_FIND(0, MAX_ITEM_OPTIONS, k, items[i].option[k].index != item.option[k].index);


Isn't this supposed to check that both index and value match?

Fixed, thanks.

MishimaHaruna · 2017-03-17T11:03:49Z

src/map/clif.h

@@ -1351,7 +1351,7 @@ struct clif_interface {
 	void (*pNPCMarketClosed) (int fd, struct map_session_data *sd);
 	void (*pNPCMarketPurchase) (int fd, struct map_session_data *sd);
 	/* */
-	void (*add_random_options) (unsigned char* buf, struct item* item);
+	int (*add_random_options) (struct ItemOptions *buf, const struct item *it);


since we're calling them Item Options rather than Random Options, I think this method should also be renamed accordingly

MishimaHaruna · 2017-03-17T11:14:58Z

src/map/itemdb.c

+		}
+
+		/* Set Constant */
+		script->set_constant(str, t_opt.index, false, false);


What happens when you reload the item DB? You might want to use script->set_constant2() instead, that allows redefinition as long as the value doesn't change.

Changed to script->set_constant2(). Thanks for the clarification.

MishimaHaruna · 2017-03-17T11:15:52Z

src/map/itemdb.c

+		const char *str = NULL;
+
+		if (!libconfig->setting_lookup_int16(conf, "Id", &t_opt.index) || t_opt.index <= 0) {
+			ShowError("itemdb_read_options: Invalid Option Id provided for entry %d in '%s', skipping...\n", t_opt.index, filepath);


What if it's a duplicated ID?

MishimaHaruna · 2017-03-17T11:34:37Z

src/map/script.c

+		i = pc->checkequip(sd, script->equip[equip_index - 1]);
+	}
+
+	if (i >= 0 && sd->status.inventory[i].nameid) {


And else? The documentation above says that I should expect -1 on failure

Fixed, thank you.

MishimaHaruna · 2017-03-17T11:40:46Z

src/map/status.c

+
+		if (index >= 0 && sd->inventory_data[index]) {
+			int j;
+			struct item_option *ito = NULL;


Move this to inside the for loop?

MishimaHaruna · 2017-03-17T11:42:12Z

src/map/status.c

+			int j;
+			struct item_option *ito = NULL;
+			for (j = 0; j < MAX_ITEM_OPTIONS; j++) {
+				short option_index = sd->status.inventory[index].option[j].index;


Any reason to use short here instead of int?

Changed to a more appropriate uint16, the data-type for index in struct option in struct item (mmo.h)

MishimaHaruna · 2017-03-17T11:42:46Z

src/map/status.c

+
+				if ((ito = itemdb->option_exists(option_index)) == NULL || ito->script == NULL)
+					continue;
+				else


This else looks very redundant after the continue

true, removed.

MishimaHaruna · 2017-03-17T11:46:33Z

src/map/status.c

+					return 1;
+			}
+		}
+		status->current_equip_option_index = -1;


Redundant. It's already cleared at the beginning of the loop. If you want to clean it up at the end, move this two lines below, after the end of the loop.
Also, does status->current_equip_index not need to be cleared?

Moved two lines below along with the clearing statement for status->current_equip_item_index

sagunkho

@MishimaHaruna updated with changes.

PS. For now I have not touched the buying/searchstore functions as the cards are set and passed on from the searchstore packet in clif_search_store_info_ack and we'll have to wait until information on the options data in the packet is discovered.

sagunkho · 2017-03-27T14:55:44Z

src/map/script.c

+	int i = -1;
+	struct map_session_data *sd = script->rid2sd(st);
+
+	if (sd == NULL) {


sagunkho · 2017-03-27T14:58:45Z

src/map/script.c

+		i = pc->checkequip(sd, script->equip[equip_index - 1]);
+	}
+
+	if (i >= 0 && sd->status.inventory[i].nameid) {


Fixed, thank you.

sagunkho · 2017-03-27T14:59:11Z

src/map/status.c

+
+		if (index >= 0 && sd->inventory_data[index]) {
+			int j;
+			struct item_option *ito = NULL;


sagunkho · 2017-03-27T15:02:53Z

src/map/status.c

+			int j;
+			struct item_option *ito = NULL;
+			for (j = 0; j < MAX_ITEM_OPTIONS; j++) {
+				short option_index = sd->status.inventory[index].option[j].index;


Changed to a more appropriate uint16, the data-type for index in struct option in struct item (mmo.h)

sagunkho · 2017-03-27T15:03:38Z

src/map/status.c

+
+				if ((ito = itemdb->option_exists(option_index)) == NULL || ito->script == NULL)
+					continue;
+				else


true, removed.

sagunkho · 2017-03-27T15:04:45Z

src/map/script.c

+	int val = 0, type = script_getnum(st, 2);
+	struct map_session_data *sd = NULL;
+
+	if ((sd = script->rid2sd(st)) == NULL || status->current_equip_item_index == -1


Fixed, thanks.

sagunkho · 2017-03-27T15:05:00Z

src/map/itemdb.c

 	itemdb->destroy_item_data(&itemdb->dummy, 0);
 	db_destroy(itemdb->names);
 }

 void do_init_itemdb(bool minimal) {
 	memset(itemdb->array, 0, sizeof(itemdb->array));
 	itemdb->other = idb_alloc(DB_OPT_BASE);
+	itemdb->options = idb_alloc(DB_OPT_RELEASE_BOTH);


Fixed, thanks.

sagunkho · 2017-03-27T15:05:40Z

src/map/itemdb.c

+		}
+
+		/* Set Constant */
+		script->set_constant(str, t_opt.index, false, false);


Changed to script->set_constant2(). Thanks for the clarification.

sagunkho · 2017-03-27T17:01:27Z

src/map/itemdb.c

+		}
+
+		if (!libconfig->setting_lookup_string(conf, "Name", &str)) {
+			ShowError("itemdb_read_options: Invalid Option variable name provided for Option Id %d in '%s', skipping...\n", t_opt.index, filepath);


Fixed, thanks.

sagunkho · 2017-03-27T17:02:16Z

src/char/char.c

 				// They are the same item.
 				ARR_FIND(0, MAX_SLOTS, j, items[i].card[j] != item.card[j]);
-				if (j == MAX_SLOTS
+				ARR_FIND(0, MAX_ITEM_OPTIONS, k, items[i].option[k].index != item.option[k].index);


Fixed, thanks.

MishimaHaruna · 2017-04-03T08:24:20Z

src/map/script.c

+		return false;
+	}
+
+	if (equip_index < 0) {


This check happens twice (see line 13653). This one should be removed

MishimaHaruna · 2017-04-03T08:24:44Z

src/map/script.c

+		return false;
+	}
+
+	if (equip_index < 0) {


Duplicated check here as well (line 13726)

MishimaHaruna · 2017-04-03T08:25:42Z

src/map/script.c

+		return false;
+	}
+
+	if (sd->status.inventory[i].nameid) {


Please consider using an explicit != 0 when checking non-boolean numeric variables

MishimaHaruna · 2017-04-03T08:32:18Z

src/map/script.c

+		clif->refine(sd->fd, 0, i, sd->status.inventory[i].refine); // notify client of a refine.
+		clif->delitem(sd, i, 1, DELITEM_MATERIALCHANGE); // notify client to simulate item deletion.
+		/* Log the Item */
+		logs->pick_pc(sd, LOG_TYPE_SCRIPT, 1, &sd->status.inventory[i], sd->inventory_data[i]);


Logs should happen in pairs, there should be a matching entry with -1 before (see BUILDIN(successrefitem) for reference)

MishimaHaruna · 2017-04-03T08:35:01Z

src/map/status.c

+		if (index >= 0 && sd->inventory_data[index]) {
+			int j = 0;
+			for (j = 0; j < MAX_ITEM_OPTIONS; j++) {
+				uint16 option_index = sd->status.inventory[index].option[j].index;


sd->status.inventory[index].option[j].index is int16, not uint16

MishimaHaruna · 2017-04-03T08:37:43Z

src/map/storage.c

-		for (i = 0; i < MAX_SLOTS && (a->card[i] == b->card[i]); i++);
-		return (i == MAX_SLOTS);
+		int i = 0, k = 0;
+		ARR_FIND(0, MAX_SLOTS, i, a->card[i] == b->card[i]);


Did you mean !=?

Oops. Yes, fixed.

MishimaHaruna · 2017-04-03T10:00:26Z

Thank you, all clear from my side. Looks good to merge once rebased, if @4144 approves

Emistry

the value seem like different from others due to the naming of inventory and cart_inventory, perhaps centralize it in a way ?

Emistry · 2017-04-03T10:22:35Z

src/map/script.c

@@ -13706,7 +13919,7 @@ BUILDIN(petloot)
 BUILDIN(getinventorylist)
 {
 	struct map_session_data *sd = script->rid2sd(st);
-	char card_var[NAME_LENGTH];
+	char card_var[26];


using a new define constant? or something like char card_var[NAME_LENGTH + 10]; ?

Emistry · 2017-04-03T10:22:45Z

src/map/script.c

@@ -13739,7 +13960,7 @@ BUILDIN(getinventorylist)
 BUILDIN(getcartinventorylist)
 {
 	struct map_session_data *sd = script->rid2sd(st);
-	char card_var[26];
+	char card_var[30];


using a new define constant? or something like char card_var[NAME_LENGTH + 10]; ?

it's not really NAME_LENGTH -- it should rather be SCRIPT_VARNAME_LENGTH, since it's a variable name

Fixed, it is now of SCRIPT_VAR_LENGTH.

4144 · 2017-04-04T00:05:45Z

src/map/itemdb.c

+
+		/* Store ptr in the database */
+		idb_put(itemdb->options, t_opt.index, s_opt);
+


Here need call additional function for read custom plugin option flags. Same like in itemdb.

Woops, forgot the call the function 🤣

dastgirp · 2017-04-04T04:23:04Z

sql-files/main.sql

@@ -79,6 +79,16 @@ CREATE TABLE IF NOT EXISTS `auction` (
  `card1` SMALLINT(11) NOT NULL DEFAULT '0',
  `card2` SMALLINT(11) NOT NULL DEFAULT '0',
  `card3` SMALLINT(11) NOT NULL DEFAULT '0',
+  `opt_idx0` SMALLINT(5) UNSIGNED NOT NULL DEFAULT '0',
+  `opt_val0` SMALLINT(5) UNSIGNED NOT NULL DEFAULT '0',


I guess val shouldn't be unsigned?

Yes, fixed. Thanks.

Allows the infusing of equipments with bonus item options. This feature is constrained to clients of packet versions greater than or equal to `20150226`. Item Options and their effects are defined server-side in `db/item_options.conf` and client side in `data/luafiles514/lua files/datainfo/addrandomoptionnametable.lub` The ID of the option must tally with the correct index of the description provided in the client side lua file to avoid bugs. IT_OPT_* keys and MAX_ITEM_OPTIONS macro are also exported from the source as constants. An additional flag `disable_options` has been added to sql, and as `DisableOptions: true/false (boolean, defaults to false !!for equipments only!!)` to item_db.conf files. Script commands documentation is also included. SQL file updates are included. Credits: [Smokexyz](https://github.com/Smokexyz) Style and Script Fixes by [Asheraf](https://github.com/Asheraf) Initial design Idea by [secretdataz](https://github.com/secretdataz)

4144 · 2017-04-08T16:29:44Z

I think from my side clear too

MishimaHaruna · 2017-04-09T13:24:19Z

Ok, let's merge this then, thank you!

MishimaHaruna added the status:code-review Awaiting code review label Mar 2, 2017

sagunkho force-pushed the IRO branch 2 times, most recently from 34da8c1 to 8592825 Compare March 2, 2017 12:02

dastgirp reviewed Mar 2, 2017

View reviewed changes

Jedzkie reviewed Mar 2, 2017

View reviewed changes

db/item_options.conf Outdated

{

Id: 3

Name: "VAR_STRAMOUNT"

Script: <" bonus bStr,getequippedoptioninfo(IT_OPT_VALUE); ">

Copy link

Contributor

Jedzkie Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

Jedzkie reviewed Mar 2, 2017

View reviewed changes

4144 requested changes Mar 2, 2017

View reviewed changes

sagunkho added the status:inprogress Issue is being worked on / the pull request is still a WIP label Mar 2, 2017

sagunkho force-pushed the IRO branch from 8592825 to 9dbaaa3 Compare March 2, 2017 19:41

sagunkho force-pushed the IRO branch from 9dbaaa3 to 6ff6a2c Compare March 2, 2017 19:59

Asheraf reviewed Mar 2, 2017

View reviewed changes

Ridley8819 reviewed Mar 2, 2017

View reviewed changes

Emistry reviewed Mar 3, 2017

View reviewed changes

sagunkho force-pushed the IRO branch 2 times, most recently from 00210a7 to b9e4a1d Compare March 3, 2017 13:04

sagunkho force-pushed the IRO branch 2 times, most recently from eaaa2f2 to 59a935a Compare March 5, 2017 08:55

sagunkho force-pushed the IRO branch from d90216f to 01fd9c8 Compare March 6, 2017 12:43

sagunkho removed the status:inprogress Issue is being worked on / the pull request is still a WIP label Mar 9, 2017

4144 requested changes Mar 12, 2017

View reviewed changes

sagunkho force-pushed the IRO branch from 01fd9c8 to 0476e48 Compare March 13, 2017 11:29

MishimaHaruna reviewed Mar 16, 2017

View reviewed changes

sagunkho force-pushed the IRO branch from 0476e48 to c13fa07 Compare March 16, 2017 19:46

MishimaHaruna requested changes Mar 17, 2017

View reviewed changes

sagunkho force-pushed the IRO branch from bade92a to 0efc20a Compare March 27, 2017 17:04

sagunkho commented Mar 27, 2017

View reviewed changes

sagunkho force-pushed the IRO branch 2 times, most recently from 6711162 to ffae6ac Compare March 28, 2017 16:34

MishimaHaruna requested changes Apr 3, 2017

View reviewed changes

sagunkho force-pushed the IRO branch from fd077a8 to 157eff1 Compare April 3, 2017 09:43

MishimaHaruna approved these changes Apr 3, 2017

View reviewed changes

Emistry reviewed Apr 3, 2017

View reviewed changes

4144 requested changes Apr 4, 2017

View reviewed changes

sagunkho force-pushed the IRO branch from 157eff1 to 6b0c853 Compare April 4, 2017 00:59

dastgirp reviewed Apr 4, 2017

View reviewed changes

sagunkho force-pushed the IRO branch 3 times, most recently from ffae6ac to 974222a Compare April 4, 2017 09:06

4144 approved these changes Apr 8, 2017

View reviewed changes

MishimaHaruna merged commit db5a1d0 into HerculesWS:master Apr 9, 2017

MishimaHaruna removed the status:code-review Awaiting code review label Apr 9, 2017

MishimaHaruna mentioned this pull request Apr 9, 2017

Size of struct mmo_charstatus #1669

Closed

4 tasks

AnnieRuru mentioned this pull request Feb 19, 2019

Missing several features for Random Option #2379

Open

9 tasks

		@@ -0,0 +1,234 @@
		//================= Hercules Script =======================================


		/* Set Script */
		t_opt.script = *str ? script->parse(str, filepath, t_opt.index, SCRIPT_IGNORE_EXTERNAL_BRACKETS, NULL) : NULL;


		/* Store ptr in the database */
		idb_put(itemdb->options, t_opt.index, s_opt);

Implementation of Item Options System. #1598

Implementation of Item Options System. #1598

Conversation

sagunkho commented Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagunkho Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

secretdataz commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jedzkie Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagunkho Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

FlippAcademy commented Mar 2, 2017

sagunkho commented Mar 2, 2017

4144 commented Mar 2, 2017

sagunkho commented Mar 2, 2017

sagunkho commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagunkho Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagunkho Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagunkho Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagunkho Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagunkho Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagunkho Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emistry commented Mar 3, 2017 • edited Loading

Database

Script Command

Choose a reason for hiding this comment

sagunkho Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagunkho commented Mar 3, 2017 • edited Loading

sagunkho commented Mar 3, 2017 • edited Loading

Atemo commented Mar 3, 2017

sagunkho commented Mar 2, 2017 •

edited

Loading

sagunkho Mar 2, 2017 •

edited

Loading

Jedzkie Mar 2, 2017 •

edited

Loading

sagunkho Mar 2, 2017 •

edited

Loading

sagunkho Mar 3, 2017 •

edited

Loading

sagunkho Mar 3, 2017 •

edited

Loading

sagunkho Mar 3, 2017 •

edited

Loading

sagunkho Mar 3, 2017 •

edited

Loading

sagunkho Mar 3, 2017 •

edited

Loading

sagunkho Mar 3, 2017 •

edited

Loading

Emistry commented Mar 3, 2017 •

edited

Loading

sagunkho Mar 3, 2017 •

edited

Loading

sagunkho commented Mar 3, 2017 •

edited

Loading

sagunkho commented Mar 3, 2017 •

edited

Loading

sagunkho commented Mar 3, 2017 •

edited

Loading

sagunkho Mar 27, 2017 •

edited

Loading