Skip to content
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

Adds BUILDIN(equipidx); BUILDIN(equip) now returns 0 or 1. #2355

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bWolfie
Copy link
Contributor

@bWolfie bWolfie commented Jan 22, 2019

Pull Request Prelude

Changes Proposed

  1. Adds new buildin equipidx().
    i.e. equipidx(<index>{, <item id>})

The reason for this change is it allows you the option to choose the specific inventory index to equip. Of course, you need to know the inventory index.

Currently I am using this in a mass refiner script. Since you need to equip an item to refine it, I was having trouble due to the equip() command always equipping the lowest index (I believe that's what it was doing).

Here is a sample in which all the different versions of 'Knife' are cycled through and equipped every 1 second.

.@id = Knife;
getinventorylist();
for (.@i = 0; .@i < @inventorylist_count; .@i  ) {
	if (@inventorylist_id[.@i] == .@id) {
		.@Index[.@j] = .@i;
		.@j  ;
		.@count  ;
	}
}

for (.@i = 0; .@i < .@count; .@i  ) {
	equipidx(.@Index[.@i], .@id);
	sleep2(1000);
}
end;
  1. equip() now returns 0 or 1 depending on whether the item successfully equipped.

Issues addressed:
N/A

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@bWolfie bWolfie force-pushed the buildin_equip branch 2 times, most recently from 1e0462d to 06bcc68 Compare January 22, 2019 15:05
@AnnieRuru
Copy link
Contributor

I think don't need, *equip2 already seems enough

Currently I am using this in a mass refiner script`

... this is not an official feature ...

Since you need to equip an item to refine it, I was having trouble due to the equip() command always equipping the lowest index (I believe that's what it was doing)

I am sure *successrefitem is meant to upgrade the equipped item ...

well, just my 2 cents

PS: when you say mass refiner script, I was thinking about using delitem2/getitem2
hmmm ... we don't have getitem3 ??

@4144
Copy link
Contributor

4144 commented Jan 22, 2019

i not sure probably better create command equipIdx? Because if you know index, id is useless

@AnnieRuru
Copy link
Contributor

erm ... but if the item already refined, it went from knife 0 into knife 10
isn't the index changed ? because its entirely different item

@4144
Copy link
Contributor

4144 commented Jan 22, 2019

if get list non refined items before start refining, item with this it cant be refined in this loop two times.

I mean one item refine do remove/add item, and it reusing existing item index or adding item in new index, it not affect other items in one loop iteration

@4144
Copy link
Contributor

4144 commented Jan 22, 2019

Another option for refine without add/remove item is abusing packet ZC_ENCHANT_EQUIPMENT script command enchantitem.
Normally it only add/remove cards in equipped item. But i can abuse it for change almost any items properties. Is this feature should be added?

@AnnieRuru
Copy link
Contributor

that sounds AWESOME ...

but wait, can it change the ItemID as well ?
like change from knife into Spear ...? without deleting existing item ?
seems like it can cause weight issue or something

struct PACKET_ZC_ENCHANT_EQUIPMENT {
	int16 packetType;
	int16 wearState;
	int16 cardSlot;
#if PACKETVER_MAIN_NUM >= 20181121 || PACKETVER_RE_NUM >= 20180704 || PACKETVER_ZERO_NUM >= 20181114
	int32 itemId;
#else
	int16 itemId;
#endif
	int8 equipFlag;
} __attribute__((packed));

enlighten me, but I didn't see the refine flag there, nor the random option ...

@4144
Copy link
Contributor

4144 commented Jan 22, 2019

item id i think can be changed too. But i not sure is client update all things if change id.
As i said before, for change other fields than cards need abuse client features.
With item options bit hard, because need send many packets for change them.
All magic happend in client if use cardSlot smaller or bigger than 0 to 3.

@bWolfie
Copy link
Contributor Author

bWolfie commented Jan 23, 2019

After successrefitem(), it seems the item doesn't change index and I can continue to loop through all them in my inventory.

Here's a video of me using the mass refiner with this new equip function: https://www.youtube.com/watch?v=J8C3DxtlhN4

equipidx() is a good suggestion (@4144). Maybe that is a better way to go.

@bWolfie bWolfie force-pushed the buildin_equip branch 2 times, most recently from a525f09 to f03ab26 Compare January 23, 2019 06:19
@bWolfie bWolfie changed the title Allows BUILDIN(equip) to specify an inventory index to equip. Adds BUILDIN(equipidx) and equip() returns 0 or 1. Jan 23, 2019
@bWolfie bWolfie changed the title Adds BUILDIN(equipidx) and equip() returns 0 or 1. Adds BUILDIN(equipidx) and BUILDIN(equip) returns 0 or 1. Jan 23, 2019
@bWolfie bWolfie changed the title Adds BUILDIN(equipidx) and BUILDIN(equip) returns 0 or 1. Adds BUILDIN(equipidx); BUILDIN(equip) now returns 0 or 1. Jan 23, 2019
Copy link
Contributor

@AnnieRuru AnnieRuru left a comment

Choose a reason for hiding this comment

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

The idea of having *equipidx looks fine, but your refiner script still can be improve (based on the video)
can just loop the percentage with *getequiprefinerycnt, then just successrefitem EQI_HAND_R, N;
where N is the number of times it refines

doc/script_commands.txt Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
@bWolfie
Copy link
Contributor Author

bWolfie commented Jan 23, 2019

I added a check so index isn't out of range. Is this correct or unnecessary?

	if (i < 0 || i >= sd->status.inventorySize) {
		ShowError("buildin_equipidx: Index (%d) should be from 0-%d.\n", i, sd->status.inventorySize);
		script_pushint(st, 0);
		return false;
	}

@4144
Copy link
Contributor

4144 commented Jan 23, 2019

check is correct

src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
Copy link
Contributor

@AnnieRuru AnnieRuru left a comment

Choose a reason for hiding this comment

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

I was just about to review, 4144 is faster than me XD
anyway I have tested this in-game and made some changes

/**
 * equipidx(<index>)
 */
static BUILDIN(equipidx)
{
	int nameid = 0, i = script_getnum(st, 2);
	struct item_data *item_data;
	struct map_session_data *sd = script->rid2sd(st);

	if (sd == NULL) {
		script_pushint(st, 0);
		return true;
	}

	if (i < 0 || i >= sd->status.inventorySize) {
		ShowError("buildin_equipidx: Index (%d) should be from 0-%d.\n", i, sd->status.inventorySize - 1);
		script_pushint(st, 0);
		return false;
	}

	if (sd->status.inventory[i].equip != 0) { // item already equipped, run silently
		script_pushint(st, 1);
		return true;
	}

	nameid = sd->status.inventory[i].nameid;
	if ((item_data = itemdb->exists(nameid)) == NULL) {
		ShowError("buildin_equipidx: Invalid Item ID (%d).\n", nameid);
		script_pushint(st, 0);
		return false;
	}

	if (pc->equipitem(sd, i, item_data->equip) == 0) {
		ShowWarning("buildin_equipidx: Item ID (%d) at index (%d) cannot be equipped.\n", nameid, i);
		script_pushint(st, 0);
		return false;
	}

	script_pushint(st, 1);
	return true;
}

honestly I don't see the point having {, } optional field there
after all, this is run by *getinventorylist,
and it already can be check by @inventorylist_id[.@i] already isn't it ?

src/map/script.c Outdated Show resolved Hide resolved
@AnnieRuru
Copy link
Contributor

AnnieRuru commented Jan 23, 2019

another things concern me is assassin class can equip weapons in both hands,
accessory can equip in EQI_ACC_L or EQI_ACC_R

hmm ... if you couldn't do it, can leave at it is, after all nobody actually complain about this issue

*equip(<item id>{, <equip position>})

@bWolfie
Copy link
Contributor Author

bWolfie commented Jan 24, 2019

Thanks for the input guys. I am busy for a few days I will update once I'm free. Also, thanks for your tips on the refiner script, AnnieRuru. Was able to streamline it so script is cleaner and effects are only displaying once for better feel for the player.

@bWolfie bWolfie force-pushed the buildin_equip branch 3 times, most recently from dca185b to 698dd34 Compare January 29, 2019 12:50
@bWolfie
Copy link
Contributor Author

bWolfie commented Jan 29, 2019

Updated based on comments from Annie and 4144.

Though I made this bit if (pc->equipitem(sd, i, item_data->equip) == 0) { return true instead of false, since I don't see reason to stop script execution if it failed to equip (we're already returning 0 in that instance).

src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
@bWolfie
Copy link
Contributor Author

bWolfie commented Jan 30, 2019

Okay done. Let me know if anything else is needed.

@Emistry Emistry added component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder labels Feb 26, 2019
doc/script_commands.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@AnnieRuru AnnieRuru left a comment

Choose a reason for hiding this comment

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

ok wait 4144 again

return true;
}

nameid = script_getnum(st, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

nameid can be defined here
int nameid = script_getnum(st, 2);

@AnnieRuru
Copy link
Contributor

/*=================================================
 * Checks if the player can equip the item at index n in inventory.
 * Returns 0 (no) or 1 (yes).
 *------------------------------------------------*/
static int pc_isequip(struct map_session_data *sd, int n)

try introduce a new script command *canequip to check if that item can be equip ?

getinventorylist();
for (.@i = 0; .@i < @inventorylist_count; .@i) {
if (@inventorylist_id[.@i] == Knife && @inventorylist_expire[.@i]) {
equipidx(.@i);
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks to dastgir idea, that just push another array of the index,
that means this example has to change accordingly

Suggested change
equipidx(.@i);
equipidx(@inventorylist_idx[.@i]);

@@ -5622,6 5625,30 @@ Examples:

---------------------------------------

*equipidx(<index>)

The equipidx function will attempt to equip the item at the given inventory
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The equipidx function will attempt to equip the item at the given inventory
This function will attempt to equip the item at the given

inventory index. This feature is only really useful when multiple instances
of an Item ID exist in the player's inventory.

Returns 1 if the item was successfully equipped.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Returns 1 if the item was successfully equipped.
Return true if successful, otherwise it will return false.

@@ -5603,6 5603,9 @@ item in his/her inventory, while the autoequip function will equip the
given item ID when this is looted. The option parameter of the autoequip
is 1 or 0, 1 to turn it on, and 0 to turn it off.

equip() returns 1 if the item was successfully equipped and 0 if the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
equip() returns 1 if the item was successfully equipped and 0 if the
Return equipment's position if successful, otherwise it will return false.

@@ -25491,6 25540,7 @@ static void script_parse_builtin(void)
BUILDIN_DEF(equip,"i"),
BUILDIN_DEF(autoequip,"ii"),
BUILDIN_DEF(equip2,"iiiiiii"),
BUILDIN_DEF(equipidx, "i?"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
BUILDIN_DEF(equipidx, "i?"),
BUILDIN_DEF(equipidx, "i"),

didnt have optional parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants