Update stylist.txt #2357

AnnieRuru · 2019-01-24T23:43:44Z

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 will be closed if the above-mentioned criteria are not fulfilled.

Issues addressed

#2356

Changes Proposed

Update stylist.txt

fix ID start with 0 instead of 1
add Job_Summoner to have its own specific range

Affected Branches

Master

Known Issues and TODO List

HerculesWSAPI · 2019-01-24T23:43:49Z

This change is

AnnieRuru · 2019-01-25T10:15:50Z

yeah this looks better, the CVS style makes the script hard to read

I leave it here in case anyone love to have a look
https://drive.google.com/file/d/1fZipfK19BQ-99IG6T7Ww9fLYau1LhlfW/view

dastgirp

Everything seems good. Just a minor newline adjustment required

npc/custom/stylist.txt

Asheraf · 2019-01-28T18:58:09Z

npc/custom/stylist.txt

+	.maxstyles[LOOK_HAIR_COLOR] = getbattleflag("max_hair_color");
+	.maxstyles[LOOK_CLOTHES_COLOR] = getbattleflag("max_cloth_color");
+
+	.maxstyles[Job_Summoner + LOOK_HAIR] = getbattleflag("max_hair_style");


4218+1, im not sure if the scripting engine actually allocates all of this but sounds like a bad practice for people that may look into this script for reference, I would suggest using the job id in the array name and use getd for getting the value.

yes, the script engine can parse it fine,
after Ind upgrade script engine years ago, our array become a pointer

you mean like

setd ".maxstyles"+ Job_Summoner +"["+ LOOK_HAIR +"]", getbattleflag("max_hair_style");

setting a bad practice ... you have a point

Emistry · 2019-01-29T01:42:02Z

imo, instead of start at 0, perhaps we should consider min_hair_style or min_hair_color instead? not everyone may start with 0 for their own preference.
fully utilize these settings?
https://github.com/HerculesWS/Hercules/blob/stable/conf/map/battle/client.conf#L45-L53

AnnieRuru · 2019-01-30T04:36:45Z

@Asheraf
The other way of not using setd/getd is use function

@Emistry
yes, I saw n0tttt script's hairstyle start with 1 instead of 0

Emistry

Just saying ...
encourage better script practices? not to miss any of those end; / close;. Member who're not aware of these may have issues with the script when they edit it?

Emistry · 2019-01-30T07:00:23Z

npc/custom/stylist.txt

+	if (BaseClass != Job_Summoner)
+		callsub(L_styles, .@part, .minstyle[.@part], .maxstyle[.@part]);
+	else
+		callsub(L_styles, .@part, .summoner_minstyle[.@part], .summoner_maxstyle[.@part]);


missing end; ?

Suggested change

callsub(L_styles, .@part, .summoner_minstyle[.@part], .summoner_maxstyle[.@part]);

callsub(L_styles, .@part, .summoner_minstyle[.@part], .summoner_maxstyle[.@part]);

end;

https://github.com/rathena/rathena/blob/master/npc/custom/battleground/bg_emp.txt#L79

you see, when I actually submit this script to Euphy,
I consider a little bit about changing my usual scripting style to suit rathena general behavior

OnRedDown: callsub L_EmpDown, "Red", .blue; end OnBlueDown: callsub L_EmpDown, "Blue", .red; end; L_EmpDown: mapannounce "bat_a01", strcharinfo(0) +" has destroyed "+ getarg(0) +" Team's Emperium.", bc_map; .winside = getarg(1); awake strnpcinfo(0); return;

this feels like OnRedDown and OnBlueDown has its own operation and nothing to do with each other

OnRedDown: callsub L_EmpDown, "Red", .blue; end; OnBlueDown: callsub L_EmpDown, "Blue", .red; end; L_EmpDown: mapannounce "bat_a01", strcharinfo(0) +" has destroyed "+ getarg(0) +" Team's Emperium.", bc_map; .winside = getarg(1); awake strnpcinfo(0); return;

I am wondering what the end; is doing there

in the end, this 1 line method was accepted not only by Euphy, but also lots of rAthena developers
rathena/rathena#3025

I'm quite sure if the callsub was meant never has a return; there's no need for an end; there

Emistry · 2019-01-30T07:02:55Z

npc/custom/stylist.txt

+			input(.@i, .@minstyle, .@maxstyle);
+			break;
+		case 4:
+			.@i = .@revert;


missing break;?

Suggested change

.@i = .@revert;

.@i = .@revert;

break;

I read a few c++ guide, most guide says there's no need to have break at the last case statement

although I admit I didn't have a default case there

Just for good practices/habits.
Since most of the cases look similar, its good to have the break; for each of them.
Protect yourself from doing thing wrongly when you out of a sudden re-ordering the cases in future.

Emistry · 2019-01-30T07:15:02Z

npc/custom/stylist.txt

 		}
 	}
+	end;
+OnInit:


Personally, I would prefer these:

Suggested change

OnInit:

setarray .look[0], LOOK_HAIR, LOOK_HAIR_COLOR, LOOK_CLOTHES_COLOR;

.minstyle[.look[0]] = .summoner_minstyle[.look[0]] = getbattleflag("min_hair_style");

.maxstyle[.look[0]] = .summoner_maxstyle[.look[0]] = getbattleflag("max_hair_style");

.minstyle[.look[1]] = .summoner_minstyle[.look[1]] = getbattleflag("min_hair_color");

.maxstyle[.look[1]] = .summoner_maxstyle[.look[1]] = getbattleflag("max_hair_color");

.minstyle[.look[2]] = .summoner_minstyle[.look[2]] = getbattleflag("min_cloth_color");

.maxstyle[.look[2]] = .summoner_maxstyle[.look[2]] = getbattleflag("max_cloth_color");

end;

if in the future there are any changes to the constant, I would only need to change 1 line....
cons: it may make the array harder to read/understand.

.minstyle[.look[0]] = .summoner_minstyle[.look[0]] = getbattleflag("min_hair_style");

totally defeats the purpose of this script

I thought I post in the issue,
explain our stylist outdated because of the introduction of summoner class

if we combine them, when they need to change summoner class have different max_cloth_style,
they wonder why the normal job also change at the same time

nope, i meant this

setarray .look[0], LOOK_HAIR, LOOK_HAIR_COLOR, LOOK_CLOTHES_COLOR; .minstyle[.look[0]] = ...... .minstyle[.look[1]] = ...... .minstyle[.look[2]] = ...... .summoner_minstyle[.look[0]] = ...... .summoner_minstyle[.look[1]] = ...... .summoner_minstyle[.look[2]] = ......

beside the current emulator doesn't introduce a new setting for doram class, they are just sharing the same value at the moment.

It does, but in stylist_db.conf. In there, Doram class can only go up to hairstyle 6. However, hair_style and hair_dye npcs don't even check class. Anyways, why not put 6 as default max hairstyle for Doram, just like stylist_db.conf? This way the script would be "plug n' play" for server owners with Doram class.

that's the problem, we want the emulator to simulate official ragnarok online
and not everyone are happy with the official settings

that's why battle_config exist ... to change certain popular configuration without restarting server
that's why custom scripts exist ... script support/request is the most generate post on the forum

what I'm trying to say is, this script should emulate official settings,
but the configuration is there for server who wish to change the value
and not everyone use number 6 either, my test server crash at value no.2
and members can also download/purchase extra cloth in graphic release

AnnieRuru · 2019-01-30T09:54:02Z

by the way, I can understand I need to change my scripting style to suit HULD
#1292
#2233

but senseless, like I don't really see a point in doing like Emistry said
that will discourage people from converting scripts into HULD readable format

because we need more people to help convert scripts into HULD readable format

npc/custom/stylist.txt

- fix ID start with .min_style instead of 1 - add Job_Summoner to have its own specific range

AnnieRuru · 2019-02-03T09:18:41Z

I even make this script HULD compatible, LOL

I really wish I can promote HULD ... but the current HULD is not very usable,
so ... I just make this compatible, but wont promote HULD until it can be more user friendly

Asheraf

Looks good enough, a side note about the comments related to this being linked with stylist db, to be clear this is NOT an official script the goal of this should be to allow for customization and showcase how to do it. stylist_db.conf is made to be used ONLY with the styling shop user interface and it has client side limitation.

AnnieRuru · 2019-02-22T18:37:32Z

I just notice this script doesn't support 3rd job body style

// Valid range of dyes and styles on the client.
min_hair_style: 0
max_hair_style: 29
min_hair_color: 0
max_hair_color: 8
min_cloth_color: 0
max_cloth_color: 4
min_body_style: 0
max_body_style: 4

missing min_body_style and max_body_style

EDIT: can someone confirm Super Novice EX can change body style too ?

AnnieRuru force-pushed the 51-stylist branch from 627f687 to 15223a4 Compare January 25, 2019 10:13

AnnieRuru force-pushed the 51-stylist branch 2 times, most recently from 0eae05a to dc7aef7 Compare January 25, 2019 10:27

AnnieRuru requested review from 4144 and dastgirp January 25, 2019 10:28

dastgirp requested changes Jan 27, 2019

View reviewed changes

npc/custom/stylist.txt Outdated Show resolved Hide resolved

AnnieRuru force-pushed the 51-stylist branch from dc7aef7 to 544347c Compare January 27, 2019 15:36

dastgirp approved these changes Jan 27, 2019

View reviewed changes

Asheraf requested changes Jan 28, 2019

View reviewed changes

Emistry added the component:scripts Affecting the scripts and NPCs label Jan 29, 2019

AnnieRuru added the type:enhancement Issue describes an enhancement or feature that should be implemented label Jan 29, 2019

AnnieRuru force-pushed the 51-stylist branch from 544347c to ecd8d22 Compare January 30, 2019 04:33

AnnieRuru requested review from Asheraf, dastgirp and Emistry January 30, 2019 04:38

Emistry approved these changes Jan 30, 2019

View reviewed changes

Asheraf requested changes Jan 30, 2019

View reviewed changes

npc/custom/stylist.txt Outdated Show resolved Hide resolved

Update stylist.txt

1865395

- fix ID start with .min_style instead of 1 - add Job_Summoner to have its own specific range

AnnieRuru force-pushed the 51-stylist branch from ecd8d22 to 1865395 Compare February 3, 2019 09:09

AnnieRuru requested a review from Asheraf February 3, 2019 09:10

Asheraf approved these changes Feb 11, 2019

View reviewed changes

MishimaHaruna added this to the Release v2019.06.30 milestone Jun 30, 2019

MishimaHaruna merged commit 238cfaa into HerculesWS:master Jun 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update stylist.txt #2357

Update stylist.txt #2357

AnnieRuru commented Jan 24, 2019 •

edited

Loading

HerculesWSAPI commented Jan 24, 2019

AnnieRuru commented Jan 25, 2019

dastgirp left a comment •

edited

Loading

Asheraf Jan 28, 2019

AnnieRuru Jan 28, 2019

Emistry commented Jan 29, 2019

AnnieRuru commented Jan 30, 2019

Emistry left a comment

Emistry Jan 30, 2019

AnnieRuru Jan 30, 2019

Emistry Jan 30, 2019

AnnieRuru Jan 30, 2019

Emistry Jan 30, 2019

Emistry Jan 30, 2019

AnnieRuru Jan 30, 2019

Emistry Jan 30, 2019

sigtus Feb 5, 2019

AnnieRuru Feb 5, 2019

AnnieRuru commented Jan 30, 2019

AnnieRuru commented Feb 3, 2019

Asheraf left a comment

AnnieRuru commented Feb 22, 2019 •

edited

Loading

	callsub(L_styles, .@part, .summoner_minstyle[.@part], .summoner_maxstyle[.@part]);
	callsub(L_styles, .@part, .summoner_minstyle[.@part], .summoner_maxstyle[.@part]);
	end;

-OnInit:
+	setarray .look[0], LOOK_HAIR, LOOK_HAIR_COLOR, LOOK_CLOTHES_COLOR;
+	.minstyle[.look[0]] = .summoner_minstyle[.look[0]] = getbattleflag("min_hair_style");
+	.maxstyle[.look[0]] = .summoner_maxstyle[.look[0]] = getbattleflag("max_hair_style");
+	.minstyle[.look[1]] = .summoner_minstyle[.look[1]] = getbattleflag("min_hair_color");
+	.maxstyle[.look[1]] = .summoner_maxstyle[.look[1]] = getbattleflag("max_hair_color");
+	.minstyle[.look[2]] = .summoner_minstyle[.look[2]] = getbattleflag("min_cloth_color");
+	.maxstyle[.look[2]] = .summoner_maxstyle[.look[2]] = getbattleflag("max_cloth_color");
+	end;

Update stylist.txt #2357

Update stylist.txt #2357

Conversation

AnnieRuru commented Jan 24, 2019 • edited Loading

Pull Request Prelude

Issues addressed

Changes Proposed

Affected Branches

Known Issues and TODO List

HerculesWSAPI commented Jan 24, 2019

AnnieRuru commented Jan 25, 2019

dastgirp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emistry commented Jan 29, 2019

AnnieRuru commented Jan 30, 2019

Emistry left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnieRuru commented Jan 30, 2019

AnnieRuru commented Feb 3, 2019

Asheraf left a comment

Choose a reason for hiding this comment

AnnieRuru commented Feb 22, 2019 • edited Loading

AnnieRuru commented Jan 24, 2019 •

edited

Loading

dastgirp left a comment •

edited

Loading

AnnieRuru commented Feb 22, 2019 •

edited

Loading