Skip to content

Conversation

@AnnieRuru
Copy link
Contributor

@AnnieRuru AnnieRuru commented Jun 6, 2018

Pull Request Prelude

Issues addressed

too many members doesn't even know this exist
simply because it is not documented

Changes Proposed

Add F_MesItemInfo function to show item name with description link

Affected Branches

  • Master

Tested with

prontera,155,180,0	script	Test Instance	1_F_MARIA,{
	mes("You can <URL>Google<INFO>http://www.google.com/</INFO></URL> anything");
	.@id = Apple;
	mes(F_MesColor(C_BLUE) +"This message is now in BLUE");
	mes F_MesColor(C_BLACK);
    mesf("%sThis message is now in BLUE.", F_MesColor(C_BLUE));
	mes F_MesColor(C_BLACK);
	mes F_MesItemInfo(.@id);
	mesf "Testing %s", F_MesItemInfo(.@id);
	mes("Bring me an <ITEM>Apple<INFO>512</INFO></ITEM>.");
	mes("Bring me an "+ F_MesItemInfo(Apple) +".");
    mesf("Bring me an %s.", F_MesItemInfo(Apple));
	select getitemname(.@id), F_MesItemInfo(.@id);
	close;
}

Known Issues and TODO List

although unrelated, but my hexed client doesn't show the www.google.com properly

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@AnnieRuru AnnieRuru added type:enhancement Issue describes an enhancement or feature that should be implemented component:scripts Affecting the scripts and NPCs status:code-review Awaiting code review labels Jun 7, 2018
@AnnieRuru
Copy link
Contributor Author

bump

@Emistry , why you add item slot to the function ?
I test in-game,
example, Item ID 1201,1202,1203 are all Knife with different amount of slots
when you have these items in your inventory, it display the slot just below the item description
but when you display it inside mes, right click on it, it doesn't show the slot

@Emistry
Copy link
Member

Emistry commented Jan 22, 2019

@Emistry , why you add item slot to the function ?

hmm, I didnt modify anything to this PR?

anyways, if you're asking for opinion, I find that showing the item slot of the item make more sense and accurate to display the item. RO has too many redundant item due to the different amount of item slots.
Display the item slots of the said item help to reduce confusion among the player when interact with NPC.

it display the slot just below the item description

Previously, similar case only happen when the message in the mes() are long and resulting the alignment messed up.

but when you display it inside mes, right click on it, it doesn't show the slot

no idea, i never really paid any attention to this. If you asked me, all I need are just the item description to be displayed there in the popup.

@dastgirp
Copy link
Member

Also, can someone check with different clients?
I believe tags were changed 3 times.
ITEM, ITEML and ITEMLINK

@Emistry
Copy link
Member

Emistry commented Jan 22, 2019

use PACKETVER for checking and change the tag accordingly.

@AnnieRuru
Copy link
Contributor Author

AnnieRuru commented Jan 22, 2019

you are right
https://github.com/HerculesWS/Hercules/blob/master/npc/re/instances/OldGlastHeim.txt#L2747

test in-game

prontera,155,185,5	script	asdasdad	1_F_MARIA,{
	mes("<ITEM>Apple<INFO>512</INFO></ITEM>.");
	mes("<ITEMLINK>Apple<INFO>512</INFO></ITEMLINK>.");
	close;
}

nah
20180620Re

not sure when this happens

http://herc.ws/board/topic/15440-solved-itemlink-help-on-2015-09-16aragexeexe-client/?do=findComment&comment=85465

@4144
Copy link
Contributor

4144 commented Jan 22, 2019

I not sure is client using ITEMLINK or not, but search in clients show what this string present only in clients: from 2013-01-30 to 2015-07-22

@4144
Copy link
Contributor

4144 commented Jan 22, 2019

and ITEM present in clients from 2015-07-29 to latest

@AnnieRuru
Copy link
Contributor Author

script_commands.txt says the Navigation system was implemented on 20111010 ...
I thought its same date getting the <ITEMLINK>

tested with

prontera,155,185,5	script	asdasdad	1_F_MARIA,{
	mes(F_MesItemInfo(123));
	mes(F_MesItemInfo(512));
//	mes("<ITEMLINK>Apple<INFO>512</INFO></ITEMLINK>.");
	mes PACKETVER +"";
	mes(F_MesItemInfo(White_Knightage_Card));
	mes(F_MesItemInfo(Khali_Knightage_Card));
	close;
}

@4144
Copy link
Contributor

4144 commented Jan 22, 2019

Yes NAVI was added in 2011-10-10 clients

@AnnieRuru AnnieRuru requested a review from 4144 January 22, 2019 21:41
@AnnieRuru AnnieRuru requested a review from 4144 January 23, 2019 11:12
@Emistry
Copy link
Member

Emistry commented Jan 27, 2019

btw, how about adding the slot display too?

I find that showing the item slot of the item make more sense and accurate to display the item. RO has too many redundant item due to the different amount of item slots.
Display the item slots of the said item help to reduce confusion among the player when interact with NPC.

@AnnieRuru
Copy link
Contributor Author

AnnieRuru commented Jan 27, 2019

some time ago I was digging my old harddisk,
I found myself made this function long long time ago, (probably 4 years ago, on the PACKEVER)

function	script	F_GetItem2	{
	.@itemid = getarg(0);
	.@name$ = getitemname( .@itemid );
	if( .@name$ == "null" )
		return "^FF0000[Unknown-"+.@itemid+"]^000000";
	.@type = getiteminfo( .@itemid,2 );
	if( .@type == IT_ARMOR || .@type == IT_WEAPON ){
		.@name$ = .@name$ +" ["+getitemslots( .@itemid )+"]";
	}
	return "<ITEMLINK>"+.@name$+"<INFO>"+.@itemid+"</INFO></ITEMLINK>";
}

I couldn't test this very old hexed client right now, this old client now throw me char info size mismatch
but if I did like this way back then, this must mean long time ago, it actually does display item slot
and I test it again right now on newer client 20180620Re doesn't display item slot

maybe just do it, for the sake of 'old me'

EDIT: interesting note,
right now it display "Unknown Item",
but this function says ... "Unknown- 512"

also fix OldGlastHeim ITEMLINK display incorrectly
@AnnieRuru
Copy link
Contributor Author

finally able to connect to this old client 2013-08-07aRagexe_patched.exe
and I got this .... disappointing
itemlink

@AnnieRuru AnnieRuru requested a review from Emistry January 27, 2019 13:25
If you're using client from 2013-01-30 onwards, you can also use <ITEMLINK> to show
the item's description. Gravity changed this into <ITEM> since 2015-07-29 onwards.

mes("Bring me an <ITEM>Apple<INFO>512</INFO></ITEM>.");
Copy link
Member

Choose a reason for hiding this comment

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

I think this line shall be removed, since not every client version support the <ITEM> tag. It could be confusing.

Copy link
Contributor Author

@AnnieRuru AnnieRuru Jan 27, 2019

Choose a reason for hiding this comment

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

hard to define this, many members said its harder and harder to use older hex client
because client translation project always keep itself up to date with no backward compatibility

I have some older hexed client just simply because when I inactive, and active back
I always install newer stuffs, and the old stuffs just kept inside external harddisk

we should always encourage members to use latest client revision ... isn't it ?

and removing that line ... is just like hide this method from public

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean we should encourage them to use the F_ItemMesInfo and not <ITEM> or <ITEMLINK>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my opinion is show them both methods ...

ok 1 vote for showing 1 method (Emistry)
1 vote for showing both methods (AnnieRuru)

any one else vote for showing 1 method or showing both ?

if (.@itemname$ != "null") {
.@itemslot = getitemslots(.@item);
if (.@itemslot)
.@itemname$ = sprintf("%s [%d]", .@itemname$, .@itemslot);
Copy link
Member

Choose a reason for hiding this comment

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

the equipment slot should check for item type is EQI_ARMOR / EQI_WEAPON or not.
If its not armor/weapon then it shall not show any postfix of equipment slot in the item name.
If the item were armor/weapon then it shall show the equipment slot even if its [0] slot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I knew you going to say this, I have tested this in my getitemname2 function
all items are default to 0 slots

	Def: Defense                  (int, defaults to 0)
	Range: Attack Range           (int, defaults to 0)
	Slots: Slots                  (int, defaults to 0)
	Job: {                        (defaults to all job)
		All: true/false               (boolean, defaults to false)
		Novice: true/false            (boolean, defaults to false)
		Swordsman: true/false         (boolean, defaults to false)
		Magician: true/false          (boolean, defaults to false)

and no, items with [0] slot doesn't show in-game
please test it,

and WHY the fuck I have to upload a screenshot every time ????
sigh...
OK I upload screenshots because you think it work your way but it doesn't work like that

prontera,155,185,5	script	asdasdad	1_F_MARIA,{
	getitem 1201, 1;
	getitem 1203, 1;
	mes(F_MesItemInfo(1201));
	mes(F_MesItemInfo(1203));
	close;
}

fuck

@AnnieRuru AnnieRuru added this to the Release v2019.05.05 milestone Apr 10, 2019
@MishimaHaruna MishimaHaruna merged commit 892ed5f into HerculesWS:master May 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:scripts Affecting the scripts and NPCs status:code-review Awaiting code review type:enhancement Issue describes an enhancement or feature that should be implemented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants