Add *setfavoriteitemidx & *autofavoriteitem script commands - alter item favorite state

[Emistry]

Pull Request Prelude

• I have followed proper Hercules code styling.
• I have read and understood the contribution guidelines before making this PR.
• I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

• setfavoriteitemidx() set an item as favorite item or not
• if an item is set to favorite item, it will be moved into favorite
  tab, else move out from favorite tab.
• only non-equipped item can adjust the favorite item state.
• autofavoriteitem() will auto set an item as favorite state or not whenever obtained. (affect globally)

---

Sample Script: autofavoriteitem(<itemID>, <flag>)

prontera,155,181,4	script	Sample	4_F_KAFRA1,{
	.@item_id = 909;
	mes getitemname(.@item_id);
	// mes "auto favorite: "+getiteminfo2(.@item_id, 21);
	.@flag = select("disable favorite", "enable favorite") - 1;
	autofavoriteitem(.@item_id, .@flag);
	close;
}

Sample Script: setfavoriteitemidx(<idx>, <flag>)

prontera,155,181,4	script	Sample	4_F_KAFRA1,{
	do {
		getinventorylist;
		.@menu$ = "";
		for (.@i = 0; .@i < @inventorylist_count; .@i++) {
			.@c = (@inventorylist_card1[.@i] > 0) + (@inventorylist_card2[.@i] > 0) + (@inventorylist_card3[.@i] > 0) + (@inventorylist_card4[.@i] > 0);
			.@menu$ += "[idx- "+@inventorylist_idx[.@i]+"] " + getitemname(@inventorylist_id[.@i]);
			if (.@c) 
				.@menu$ += "["+.@c+" cards]";
			.@menu$ += " "+ (@inventorylist_favorite[.@i] ? "^0055FF- favorite^000000":"") + ":";
		}
		.@i = select(.@menu$) - 1;
		// based on inventory index
		setfavoriteitemidx(@inventorylist_idx[.@i], !@inventorylist_favorite[.@i]);
	} while (true);
	close;
}

@inventorylist_favorite[.@i] from this PR, but not really dependant on it. Since it can be just set with the value 0 or 1. Bonus if have it.

Issues addressed:
None

[HerculesWSAPI]

This change is 

[AnnieRuru]

missing nameid and amount check
search idx >= sd->status.inventorySize in Hercules\src folder for more example

	} else if (idx < 0 || idx >= sd->status.inventorySize) {
		ShowError("buildin_setfavouriteitem: Invalid inventory index %d (min: %d, max: %d).\n", idx, 0, (sd->status.inventorySize - 1));
		return false;
	} else if (sd->inventory_data[idx] == NULL || sd->inventory_data[idx].nameid <= 0 || sd->inventory_data[idx].amount <= 0) {
		ShowWarning("buildin_setfavouriteitem: Current inventory index %d has no data.\n", idx);
		return false;

something like that

[AnnieRuru]

btw, why using inventory index for this ?
https://github.com/AnnieRuru/Release/blob/master/plugins/favorite_tab/favorite_tab_0.1.c
I did it with ItemID instead of inventory index ...

hmm ... so ...
using ItemID can ... when pick up this ItemID, can immediately move to favorite tab
using inventory index ... when there are multiple ItemID ...
-> example equipment with same ID, only that equipment will move to favorite tab ...
hmmm ....

[AnnieRuru]

how about split into 3 script commands
similar to *equip *equipidx *autoequip
so there will be *setfavoriteitem *setfavoriteitemidx and *autofavoriteitem ?

[Emistry]

for itemID as parameter,
it doesn't take the correct/desired item if the character has multiple same itemID or item with different refine/itembonus/cards/etc (I am sure you had experienced same frustration in the past using the autoequip, delitem, and etc)
using getinventorylist could have avoided all that since all info of that item can be obtained and check during the script execution, it even pick the correct and actual inventory index that you intended to set to favorite state.
its possible to implement with itemID/index as parameter, but dont really see the needs for it.
for multiple new script command for the purpose of separating parameter index or itemid? I did vote for -1 for this.
imo, using itemID as parameter are less flexible/convenient than inventory index.

[AnnieRuru]

its possible to implement with itemID/index as parameter, but dont really see the needs for it.
for multiple new script command for the purpose of separating parameter index or itemid? I did vote for -1 for this.
imo, using itemID as parameter are less flexible/convenient than inventory index.

com'on, you didn't read the topic link in rathena forum
https://rathena.org/board/topic/118227-consumables-in-favorites/
Seravy-sensei wants the blue-gemstone in the favorite tab forever
this can only done with *autofovariteitem, set 717 as permanent item in favorite tab

... even if I said I can always release as plugin on the forum,
I also wish this script command name as *setfavoriteitemidx instead
to avoid confusion with my release later,
as your script command clearly only move item by inventory index

[Emistry]

setfavoriteitem I have added support for item_id, if the value provided are higher than inventory size, it will be treated as item_id.
But still, I don't recommend using itemID since its lack of accuracy to determine the item in case player has multiple same item id in their inventory.

autofavourite I have added this to auto set the item as favorite state if player obtained it.
This is actually a good idea to implement it. The use-case are way common if compared to setfavoriteitem.

Kindly help re-review the PR.
Thank you in advance.

[AnnieRuru]

I have tested ItemID 50 works

{
	Id: 50
	AegisName: "Red_Potion__"
	Name: "Red Potion__"
	Type: "IT_HEALING"
	Buy: 1
	Weight: 1
	Script: <" percentheal 100,100; ">
	Sprite: 501
},

@item 50 will create this item that can fully heal character

in other words, this patch only assume nobody is using ItemID below 500,
but actually you CAN create itemID below 500

[Emistry]

the patch check for if (nameid >= ITEMID_RED_POTION || nameid >= sd->status.inventorySize).
this is also one of the reason I don't wish it support item ID, since its silly whenever a player try to create an item that shouldn't fall below "minimum" item id, and gravity is brilliant enough to start itemid at 501. lol
FYI, there doesn't exist any MIN_ITEM_ID hence ITEMID_RED_POTION are used as an replacement for it.

EDIT:
beside, in your case, it would have throw an error like this. since it treat the itemID 50 as inventory index.

[Warning]: buildin_setfavourideteitem: Current inventory index 50 has no data.
[Debug]: Source (NPC): Sample at prontera (155,181)

elaborating possible use-case of the code changes, its irrelevant to code changes.

[AnnieRuru]

gravity is brilliant enough to start itemid at 501

LOL !! I actually want to say gravity is stupid enough to not start the ID from 1 or 0

btw, kindly dont use "request changes" to add a comment without really need anything to changes in the code? lol

yes, because we can use item_avail Sprite field to create itemID gravity not using
1-500 fall within the possible range that any server can use these ID range
means your code actually create conflicts for those server that actually uses these ID range

[Emistry]

I am leaving the restriction here since there are no official way to tell the min_item_id that a server is using, unless they self-defined the MIN_ITEM_ID or something equivalent.
Not to mention the current max_item_id support up to millions, I dont really see a point using itemID <= 500 as their custom item id.

if I to changed the code to give priority for legit item_id checking, it would lost the function to alter the state based on inventory index if an item ID exists with the same number as the inventory index.
Which at the end, resulting to force creating another equivalent script command to support it.

	// if item id are provided
	if (id != NULL) {
		idx = pc->search_inventory(sd, nameid);
		if (idx == INDEX_NOT_FOUND) {
			ShowError("buildin_setfavoriteitem: Item %d not found.\n", nameid);
			return false;
		}
	}

this is why itemID as parameter aren't preferable in the beginning, it has lack accuracy, and create unnecessary works, which originally could be avoided if they use getinventolist() to get the inventory_idx value to use with this script command.

[AnnieRuru]

Which at the end, resulting to force creating another equivalent script command to support it.

I have been saying separate into *setfavoriteitem and *setfavoriteitemidx
if your idea actually accurate, that also means the PR about *delitemidx and *equipidx shouldn't even exist
and should alter existing *delitem and *equip script command instead, which create further confusion

ok now wait 1 month until Haru come back and review this

--> #2427 (comment)
dismiss someone else review ... that's very bad manners

[MishimaHaruna]

I also think that having a command that can take either an index or a nameid is a recipe for problems.

My main concerns are:

• A customized setting, or a future Gravity update could push the minimum item ID to < 500
• A customized setting/client or a future Gravity update could push the maximum inventory size to > 500
• A malfunctioning script that expects to have an item ID in a variable could have the wrong value (or it could have a typo in a variable or constant, resulting into a 0), which could be silently accepted by the script as an inventory index, instead of throwing a runtime error and aborting the script because of an invalid nameid.

I'd suggest to accept either the nameid or the index (and have separate commands if you want to accept both). It's not nice to have several commands that do the same thing, but it seems to me the approach that's less prone to errors.

[Emistry]

Then I will just proceed with original plan, only accept inventory_index.
as discussed above, using itemID as parameter are less flexible/convenient than inventory index.

and renamed setfavoriteitem() to setfavoriteitemidx()

[Asheraf]

Following up on what Haru said:

• Gravity already have an item with id = 1

	[1] = {
		unidentifiedDisplayName = "머리띠",
		unidentifiedResourceName = "눈가리개",
		unidentifiedDescriptionName = {},
		identifiedDisplayName = "마력의 별",
		identifiedResourceName = "마력의별",
		identifiedDescriptionName = {},
		slotCount = 0,
		ClassNum = 0
	},

• Inventory Expansion feature allows for > 500 inventory size (herc still have some packet limits but client support it normally)