Adds support for <account id> param for getcharid(). Deprecates charid2rid. #2350

bWolfie · 2019-01-13T04:16:32Z

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

Adds support for account id to getcharid().
I.e.
getcharid(<type>{, "<character name>"})
getcharid(<type>{, <account id>})
Deprecates charid2rid() in favour of getcharid(CHAR_ID_ACCOUNT, <char id>).

Issues addressed:
N/A

HerculesWSAPI · 2019-01-13T04:16:37Z

This change is

…d2rid(), its functionality now merged into getcharid().

AnnieRuru

*charid2rid is still useful to retrieve the value from SQL table or item's char id field
^--- EDITED ---

from SQL
select char_id from char where online = 1;
well yes can use select account_id ...
but charid2rid return 0 when player is offline, similar to isloggedin return 0 when player is offline

signed item

Hercules/doc/script_commands.txt

Lines 5006 to 5017 in 52360f0

    
           // First, let's get an ID of a character who's name will be on the 
        
           // item. Only an existing character's name may be there. 
        
           // Let's assume our character is 'Adam' and find his ID. 
        
           .@charid = getcharid(CHAR_ID_CHAR, "Adam"); 
        
           // Now we split the character ID number into two portions with a 
        
           // binary shift operation. If you don't understand what this does, 
        
           // just copy it. 
        
           .@card3 = .@charid & 65535; 
        
           .@card4 = .@charid >> 16;

this only return char id

bWolfie · 2019-01-13T11:57:37Z

@AnnieRuru I don't understand what you're saying. This PR does not reduce functionality of getcharid(), only improve it.
.@charid = getcharid(CHAR_ID_CHAR, "Adam"); still returns the same value.

getcharid() already returns 0 if player is offline.
charid2rid() returns account id anyway, so may as well merge into the superior command.

getcharid

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

charid2rid

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

AnnieRuru · 2019-01-13T12:02:34Z

what I am saying is just don't deprecate the *charid2rid, because I have use them personally
nothing wrong with getcharid implementation =)

oops, edit that post, I word it wrongly again

getcharid() already returns 0 if player is offline.

yes, getcharid() return 0, but the signed item only return char id WITHOUT account ID
so when you need to check the account ID from char ID, if you deprecate that command, then how are you going to use it ?

getcharid(CHAR_ID_ACCOUNT, <char_id>) ????
of course doesn't work, because that field you have already insert with account ID

EDITING ---- OK I WRITE A SAMPLE SCRIPT -----

AnnieRuru · 2019-01-13T12:36:29Z

prontera,155,185,5	script	sfshdfk	1_F_MARIA,{
	getnameditem 1201, getcharid(0);
//	Use my getitemname2 function, I only write a small part of it
	getinventorylist;
	for ( .@i = 0; .@i < @inventorylist_count;   .@i ) {
		if ( @inventorylist_card1[.@i] == 255 || @inventorylist_card1[.@i] == 254 ) {
			.@charid = @inventorylist_card3[.@i];
			// now I have to convert this char ID into name 
			.@account_id = charid2rid(.@charid);
			if ( .@account_id )
				dispbottom rid2name(.@account_id)  "'s "  getitemname( @inventorylist_id[.@i] )  "[Online]";
			// now tell me how are you going to convert char ID into name IF charid2rid is deprecated ???
			else {
				query_sql "select name from `char` where char_id = "  .@charid, .@name$  "[Offline]";
				dispbottom .@name$  "'s "  getitemname( @inventorylist_id[.@i] );
			}
		}
	}
	end;
}

EDIT: --> .@account_id = getcharid(3, .@charid);
OH, you merge them into 1 single command

dastgirp · 2019-01-13T12:40:19Z

what I am saying is just don't deprecate the *charid2rid, because I have use them personally
nothing wrong with getcharid implementation =)

oops, edit that post, I word it wrongly again

getcharid() already returns 0 if player is offline.

yes, getcharid() return 0, but the signed item only return char id WITHOUT account ID
so when you need to check the account ID from char ID, if you deprecate that command, then how are you going to use it ?

getcharid(CHAR_ID_ACCOUNT, <char_id>) ????
of course doesn't work, because that field you have already insert with account ID

EDITING ---- OK I WRITE A SAMPLE SCRIPT -----

@AnnieRuru, @EyesOfAHawk has modified the getcharid, so that CHAR_ID_ACCOUNT will check for char_id and not for account id as 2nd argument

Although the PR seems fine, but let @Asheraf or @MishimaHaruna review whether it is good idea to merge these 2 functionality

AnnieRuru · 2019-01-13T12:46:30Z

ok I misread that part, approve it
*charid2rid is very very seldom use,
most of the time I actually want to change from char_id into name
so I have to use *charid2rid *rid2name together
I actually wish there is a *charid2name script command for it

actually yeah, need discussion, because getcharid(CHAR_ID_NAME, <char_id>) doesn't work
<-- that field already reserve for <account_id>
(that's the reason why I misunderstood this pull request =/ sorry about that)

MishimaHaruna · 2019-04-07T20:20:55Z

The part about getcharid(CHAR_ID_ACCOUNT, <char id>) seems confusing to me, I feel that having hidden special cases (the same command takes different IDs as argument depending on what you request to return) makes the scripts less readable. What does everyone else think?

AnnieRuru · 2019-04-09T15:59:18Z

hmm, rathena has this PR rathena/rathena#3924
also looks confusing to me ....

I actually wish there is a *charid2name script command for it

I rather have a script command that is clearly state what it is doing,
just like writing an essay which everyone understand
yeah a script command with hidden parameter make the script less readable ...

EDIT: exactly the point, it confused me/makes me misunderstood in previous posts

Kenpachi2k13 · 2020-01-11T08:03:52Z

@MishimaHaruna

What does everyone else think?

I do agree with you. In my opinion this special case unnecessary. If someone is able to pass a character ID to retrieve the corresponding account ID, he should also be able to pass the character's name instead.

bWolfie force-pushed the buildin_getcharid branch 4 times, most recently from a6a89a2 to a4595d7 Compare January 13, 2019 04:54

Adds support for <account id> param for getcharid(). Deprecates chari…

a4595d7

…d2rid(), its functionality now merged into getcharid().

AnnieRuru requested changes Jan 13, 2019

View reviewed changes

dastgirp approved these changes Jan 13, 2019

View reviewed changes

AnnieRuru approved these changes Jan 13, 2019

View reviewed changes

Emistry added component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder labels Jan 13, 2019

Kenpachi2k13 added the status:code-review Awaiting code review label Jan 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for <account id> param for getcharid(). Deprecates charid2rid. #2350

Adds support for <account id> param for getcharid(). Deprecates charid2rid. #2350

bWolfie commented Jan 13, 2019

HerculesWSAPI commented Jan 13, 2019

AnnieRuru left a comment •

edited

Loading

bWolfie commented Jan 13, 2019 •

edited

Loading

AnnieRuru commented Jan 13, 2019 •

edited

Loading

AnnieRuru commented Jan 13, 2019 •

edited

Loading

dastgirp commented Jan 13, 2019 •

edited

Loading

AnnieRuru commented Jan 13, 2019 •

edited

Loading

MishimaHaruna commented Apr 7, 2019

AnnieRuru commented Apr 9, 2019 •

edited

Loading

Kenpachi2k13 commented Jan 11, 2020

	// First, let's get an ID of a character who's name will be on the
	// item. Only an existing character's name may be there.
	// Let's assume our character is 'Adam' and find his ID.

	.@charid = getcharid(CHAR_ID_CHAR, "Adam");

	// Now we split the character ID number into two portions with a
	// binary shift operation. If you don't understand what this does,
	// just copy it.

	.@card3 = .@charid & 65535;
	.@card4 = .@charid >> 16;

Adds support for <account id> param for getcharid(). Deprecates charid2rid. #2350

Are you sure you want to change the base?

Adds support for <account id> param for getcharid(). Deprecates charid2rid. #2350

Conversation

bWolfie commented Jan 13, 2019

Pull Request Prelude

Changes Proposed

HerculesWSAPI commented Jan 13, 2019

AnnieRuru left a comment • edited Loading

Choose a reason for hiding this comment

bWolfie commented Jan 13, 2019 • edited Loading

AnnieRuru commented Jan 13, 2019 • edited Loading

AnnieRuru commented Jan 13, 2019 • edited Loading

dastgirp commented Jan 13, 2019 • edited Loading

AnnieRuru commented Jan 13, 2019 • edited Loading

MishimaHaruna commented Apr 7, 2019

AnnieRuru commented Apr 9, 2019 • edited Loading

Kenpachi2k13 commented Jan 11, 2020

AnnieRuru left a comment •

edited

Loading

bWolfie commented Jan 13, 2019 •

edited

Loading

AnnieRuru commented Jan 13, 2019 •

edited

Loading

AnnieRuru commented Jan 13, 2019 •

edited

Loading

dastgirp commented Jan 13, 2019 •

edited

Loading

AnnieRuru commented Jan 13, 2019 •

edited

Loading

AnnieRuru commented Apr 9, 2019 •

edited

Loading