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

Emoticons constant name #2527

Merged
merged 9 commits into from Nov 10, 2017
Merged

Emoticons constant name #2527

merged 9 commits into from Nov 10, 2017

Conversation

Atemo
Copy link
Contributor

@Atemo Atemo commented Oct 17, 2017

  • Addressed Issue(s): None

  • Server Mode: Renewal and Pre-renewal

  • Description of Pull Request:

This PR change the emoticons constant name to match emoticonlist from client side.
Note: the previous emoticons names are now deprecated.

… client side.

* Note: the previous emoticons names are now deprecated.

Additionally target use UNITTYPE_NPC and UNITTYPE_PC constants to specify the emotion target.
* Note: previously the value of the parameter "target" for npc was 0. UNITTYPE_NPC value is 1.
@anacondaq

This comment was marked as abuse.

@Lemongrass3110
Copy link
Member

@Anacondaqq the main part of the codebase should be covered by this pull request. The rest should be easy for each serverowner to find, because the script engine will report deprecated constant usage as soon as you load the script.

@Lemongrass3110 Lemongrass3110 added component:core A fault that lies within the main framework of rAthena component:script A fault that lies within the scripts of rAthena mode:prerenewal A fault that exists within the pre-renewal mode mode:renewal A fault that exists within the renewal mode status:code-review Pull Request that requires reviewing from other developers before being pushed to master type:maintenance Issue for refactoring rAthena labels Oct 18, 2017
Copy link
Member

@Lemongrass3110 Lemongrass3110 left a comment

Choose a reason for hiding this comment

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

Honestly I would say we should deprecate one of the two commands(emotion,unitemote) or simply merge them together. This really looks like useless code duplication just to support name lookup.

@@ -12700,36 +12700,42 @@ BUILDIN_FUNC(gvgoff3)

/*==========================================
* Shows an emoticon on top of the player/npc
* emotion emotion#, <target: 0 - NPC, 1 - PC>, <NPC/PC name>
* emotion emotion#, <target: UNITTYPE_NPC (1) - NPC, UNITTYPE_PC (0) - PC>, <NPC/PC name>
Copy link
Member

Choose a reason for hiding this comment

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

Remove the numbers please. Users should never use the values behind the constants.

@@ -18500,7 +18506,12 @@ BUILDIN_FUNC(unitemote)

emotion = script_getnum(st,3);

if(script_rid2bl(2,bl))
Copy link
Member

Choose a reason for hiding this comment

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

Please edit comment /// @see e_* in db/const.txt above this command.

Copy link
Contributor

@aleos89 aleos89 left a comment

Choose a reason for hiding this comment

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

Aside from Lemon's comments, rest looks good!

Atemo and others added 8 commits October 19, 2017 23:06
* Corrected the encodage for cutin as well in quests_moscovia
* Now accepts the game ID rather than the object name.
* Removed the UNITTYPE_* parameter.
* Deprecated script command unitemote.
* Updated all NPC to reflect this new format.
* Fixed all ET_KIK -> ET_KEK.
* Updated some remaining NPC.
* Reverted ET_KIK -> ET_KEK change
* Corrected emotion character game ID
* Reverted some emotion NPC target (for duplicates NPCs)
* Fixed incorrect variable in quests_brasilis.txt (thanks to @Lemongrass3110 !)
@Atemo Atemo merged commit ea88ea5 into master Nov 10, 2017
@Atemo Atemo deleted the cleanup/emotions branch November 10, 2017 17:35
@aleos89 aleos89 removed the status:code-review Pull Request that requires reviewing from other developers before being pushed to master label Nov 10, 2017
@mazvi
Copy link
Contributor

mazvi commented Nov 11, 2017

can you give list change for example hmm to scratch, omg to huk this will help people to find and replace the custom NPC thx

@cydh
Copy link
Contributor

cydh commented Nov 11, 2017

@mazvi
Copy link
Contributor

mazvi commented Nov 11, 2017

edited, i find it from your link sir @cydh thx very much ^^

@mazvi
Copy link
Contributor

mazvi commented Nov 11, 2017

Helo cydh can i set the emoticons deprecated to true, true? the old emoticon will be working?

@Yuchinin
Copy link

Aww changing syntax and constant? Will need to convert every emotion line by line now then@@

@anacondaq

This comment was marked as abuse.

@mazvi
Copy link
Contributor

mazvi commented Nov 11, 2017

finaly i was finished change one by one by using notepad++ use find syntax emotion :D

@Yuchinin
Copy link

@Anacondaqq Yeah there is ton. I use alot in my script. Since not just changing the constant but also the syntax. And because previously the 2rd param are for attached player and now it no more so can't just replace all with notepad++ since it might not be same behavior like before.

@Yuchinin
Copy link

Should also update the warning message? Since some don't even show the file name.

[Warning]: This constant was deprecated and could become unavailable anytime soon.
[Warning]: Usage of deprecated constant 'E_CASH'.t
[Warning]: This constant was deprecated and could become unavailable anytime soon.
[Warning]: Usage of deprecated constant 'E_AN'.
[Warning]: This constant was deprecated and could become unavailable anytime soon.
[Warning]: Usage of deprecated constant 'E_DUM'.txt
[Warning]: This constant was deprecated and could become unavailable anytime soon.

@Jeybla
Copy link
Contributor

Jeybla commented Nov 12, 2017

https://github.com/rathena/rathena/blob/master/tools/convert_emotions.py

Wrote a small conversion script. (But I didn't test the outcoming NPCs). Leave me feedback if anything is missing.

Edit:
#2598 is related to this pr.

@mazvi
Copy link
Contributor

mazvi commented Nov 12, 2017

if run script conversion @Jeybla what about line this
@cydh quote

ea88ea5#diff-d5e39e5b7e19d8ad064b046e03d14af2R2713

are this line also will be replaced?

@Cyanide0210
Copy link

I bump this because the converter is not working

@sader1992
Copy link
Contributor

@M45T3Ryu please open an issue
also i remember testing it and was working

ghost referenced this pull request Oct 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:core A fault that lies within the main framework of rAthena component:script A fault that lies within the scripts of rAthena mode:prerenewal A fault that exists within the pre-renewal mode mode:renewal A fault that exists within the renewal mode type:maintenance Issue for refactoring rAthena
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants