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

Initial release of the refine UI #2494

Open
wants to merge 21 commits into
base: master
from

Conversation

@Lemongrass3110
Copy link
Member

Lemongrass3110 commented Oct 9, 2017

  • Addressed Issue(s): None

  • Server Mode: Both

  • Description of Pull Request:
    This adds support for the new refine UI.

Open points:

  • Logic for Blacksmith Blessing
  • Limitation for HD Oridecon/Elunium/Carnium/Bradium
  • Logic for Blessed Oridecon/Elunium
  • Translation of the NPC dialog
  • Automatically disable old unused HD refine NPCs via feature check
Show resolved Hide resolved src/map/clif.cpp
.@npc_name$ = getarg(0);
.@features = getarg(1);

if( getbattleflag( "feature.refineui" ) == 3 ){

This comment has been minimized.

Copy link
@jenkijo

jenkijo Oct 9, 2017

Contributor

if( getbattleflag( "feature.refineui" ) == 2 ){

This comment has been minimized.

Copy link
@Lemongrass3110

Lemongrass3110 Oct 9, 2017

Author Member

This is fine like this.
1 = enabled in general
2 = enabled in scripts
=> so we want the system to be enabled & enabled in scripts => 3

This comment has been minimized.

Copy link
@jenkijo

jenkijo Oct 9, 2017

Contributor

Okay

This comment has been minimized.

Copy link
@jenkijo

jenkijo Oct 9, 2017

Contributor

When 2 = enabled in scripts is configured, I still can use @refineui

@jenkijo

This comment has been minimized.

Copy link
Contributor

jenkijo commented Oct 9, 2017

GRF
In GRF, I have two folders

  • One for Pre-Renewal
  • One for Renewal

I use 2017-06-14bRagexeRE client, but I still see Pre-Renewal Refine UI

UI

@Lemongrass3110

This comment has been minimized.

Copy link
Member Author

Lemongrass3110 commented Oct 9, 2017

I dont know the exact date when the refine UI was reworked again, but on 2017-09-20 it shows the new UI and works.

@jenkijo

This comment has been minimized.

Copy link
Contributor

jenkijo commented Oct 9, 2017

I have a problem:

  • Default (NPC): we can refine +1 to +6
  • With Mighty Hammer: we can refine +7 to +9
  • With Basta: we can refine +10 and up

With Refine UI, we can refine up to +20. So, other NPCs are useless. Does kRO still keep its?

@aleos89

This comment has been minimized.

Copy link
Contributor

aleos89 commented Oct 9, 2017

The NPC were removed on kRO in favor of the new UI.

Lemongrass3110 added some commits Oct 9, 2017

Added downgrade logic
Added a new configuration values in the YAML configuration to mark if a material will break or downgrade the item in case of failure.
@cydh

This comment has been minimized.

Copy link
Contributor

cydh commented on src/map/status.c in db82c45 Oct 9, 2017

So the return value of status_get_refine_cost better as struct/pointer. the usages are call this function twice only for different field

@cydh

This comment has been minimized.

Copy link
Contributor

cydh commented Oct 10, 2017

@anity99 I guess it just does the same as NPCs do

// Try to refine the item
if( materials[i].chance >= rnd() % 100 ){
// Success
item->refine = cap_value( item->refine + 1, 0, MAX_REFINE );

This comment has been minimized.

Copy link
@cydh

cydh Oct 10, 2017

Contributor

should the MAX_REFINE check before consume all requirements?

This comment has been minimized.

Copy link
@Lemongrass3110

Lemongrass3110 Oct 10, 2017

Author Member

No we want to hurt bot users. :-P

This comment has been minimized.

Copy link
@cydh

cydh Oct 10, 2017

Contributor

cmiiw, but u didn't limit the available item for refine somewhere first (here maybe? https://github.com/rathena/rathena/pull/2494/files#diff-c5d5e782db2344e04a7089b24ba6c962R20251)
I guess even a normal player still can put +20 (max refined) item to the window.
or the client has auto-acknowledge if the item is max refined?

nvm, it takes so long after clif_refineui_materials_sub check returns a false for 0-ing variables..
I need to test first LOL updating kRO files, looking for supported client

This comment has been minimized.

Copy link
@mazvi

mazvi Oct 15, 2017

Contributor

sir @cydh if you need latest kRO files, i have all, you can copy from me..

@sctnightcore sctnightcore referenced this pull request Oct 10, 2017

Closed

new refine UI ! #1252

@sctnightcore

This comment has been minimized.

Copy link
Contributor

sctnightcore commented Oct 10, 2017

I'm sorry

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 10, 2017

Is the picture below normal? Tested it without modification. When refine fails sometimes the UI doesn't update text.

To reproduce: Refine should be successful at first then 2 successive fail refine.

screenchaos001

Fixed Refine UI doesn't update the refine value on downrefine. Thanks…
… to @ecdarreola

Signed-off-by: Cydh Ramdh <cydh@pservero.com>
@ghost

This comment has been minimized.

Copy link

ghost commented Oct 10, 2017

Reference: #2494 (comment)

There's still NPC for refine in RE located in re/merchants/refine.txt. Should this be disabled also? (Vestri NPC)

@cydh cydh referenced this pull request Oct 10, 2017

Closed

Refine UI Updates #2499

@Lemongrass3110

This comment has been minimized.

Copy link
Member Author

Lemongrass3110 commented Oct 10, 2017

Vestri is still enabled on kRO. Do not ask me why though...

@cydh

This comment has been minimized.

Copy link
Contributor

cydh commented on npc/re/merchants/hd_refiner.txt in 9c155ac Oct 10, 2017

it was ugly but that case is good and at least only calls getequipweaponlv once xDD

}
OnInit: // Refine UI makes these NPCs useless
if (getbattleflag("feature.refineui") == 3)
unloadnpc strnpcinfo(0);

This comment has been minimized.

Copy link
@jenkijo

jenkijo Oct 12, 2017

Contributor

unloadnpc strnpcinfo(3);

This comment has been minimized.

Copy link
@ghost

ghost Oct 12, 2017

Map server crashes when changing the value to strnpcinfo(3); I have applied cyd's patch for refine. Will it have conflict?

Using strnpcinfo(0); map-server is normal.

@Badarosk0

This comment has been minimized.

Copy link

Badarosk0 commented Jul 11, 2018

@Lemongrass3110

When you configure refine_db.yml for EventNormalChance the extra chance is not affecting or affecting it incorrectly.
1- Edit [event_refine_chance: yes] in item.conf.
2- Set up an extra chance for a weapon of any level.
3 - Restart the server.
4- Open the refineui refine your weapon to +15.
5- Get bradium and check the chance it will show.
6- It will not show the chance configured as Event.

@mafius

This comment has been minimized.

Copy link

mafius commented Jul 13, 2018

@Lemongrass3110
sem_titulo

I am having this error when I use refineui animation it does not use item 6635, looking at an old diff had a line in refine_db.yml that BlacksmithBlessing more now does not have

@hnomkeng

This comment has been minimized.

Copy link

hnomkeng commented Aug 13, 2018

How to enable anti broken ?

@iubantot

This comment has been minimized.

Copy link

iubantot commented Sep 14, 2018

when will this be official?

@sianromantic

This comment has been minimized.

Copy link

sianromantic commented Oct 5, 2018

plz update. thank.

@Badarosk0

This comment has been minimized.

Copy link

Badarosk0 commented Oct 6, 2018

Any forecast for this update? Everyone is really looking forward to it, since the goal of donations was opened. =]

Merge branch 'master' into feature/refineui
# Conflicts:
#	conf/battle/feature.conf
#	src/map/battle.cpp
#	src/map/battle.hpp
#	src/map/clif.cpp
#	src/map/script.cpp

Haikenz referenced this pull request Oct 13, 2018

@zellkennedy

This comment has been minimized.

Copy link

zellkennedy commented Nov 7, 2018

What is missing for this became official?

@aleos89

This comment has been minimized.

Copy link
Contributor

aleos89 commented Nov 7, 2018

The open bullets in the main post.

@Everade

This comment has been minimized.

Copy link

Everade commented Dec 7, 2018

@aleos89
Can you send me the korean NPC dialog?

aleos89 added some commits Dec 7, 2018

Follow up to dc81f71
* Fixed a merge update issue.
@aleos89

This comment has been minimized.

Copy link
Contributor

aleos89 commented Dec 7, 2018

I'm not sure I've had that come across my desk yet. It's been such a long time, I'll recheck my stuff and see what I have.

@rAthenaAPI

This comment has been minimized.

Copy link
Contributor

rAthenaAPI commented Dec 7, 2018

This pull request introduces 4 alerts when merging 4df192f into 5597105 - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same

Comment posted by LGTM.com

@Asheraf

This comment has been minimized.

Copy link

Asheraf commented Dec 7, 2018

@Everade @aleos89

prt_in,63,60,0	script	홀그렌	4_M_03,{
	mes("[홀그렌]");
	mes("나는 무기와 방어구를 제련하는 대장장이라네");
	mes("자네가 장착하고 있는 아이템중에 원하는걸 제련할 수 있다네.");
	mes("어느 장비아이템을 제련하고 싶나?");
	close2();
	// openrefine();
	end;
}
@Everade

This comment has been minimized.

Copy link

Everade commented Dec 17, 2018

Professional translation:

prt_in,63,60,0	script	Hollgrehenn	4_M_03,{
	mes("[Hollgrehenn]");
	mes("I'm a blacksmith skilled in refining weapons and armors.");
	mes("I can refine an item of your choice among the items you are equipped with.");
	mes("Which item do you want to refine?");
	close2();
	// openrefine();
	end;
}
@kukas9

This comment has been minimized.

Copy link

kukas9 commented Jan 17, 2019

image
Why cant upgrade?

@Akaineko-astasi

This comment has been minimized.

Copy link

Akaineko-astasi commented Jan 17, 2019

@kukas9 you need to click the materials at the top row to choose which ones you want to use. When chosen, it looks like this:
image
The red hex is for Blacksmith_Blessing, it needs to be chosen too to be applied.

@kukas9

This comment has been minimized.

Copy link

kukas9 commented Jan 18, 2019

@Akaineko-astasi ooh, thx, forgive me the lack of attention, I thought he asked for those materials to forge, but in fact have to choose one of them hahaha

@Lemongrass3110

This comment has been minimized.

Copy link
Member Author

Lemongrass3110 commented Jan 29, 2019

Blocked until #3694 was merged.

@Busbaty

This comment has been minimized.

Copy link

Busbaty commented Feb 17, 2019

@Lemongrass3110
sem_titulo

I am having this error when I use refineui animation it does not use item 6635, looking at an old diff had a line in refine_db.yml that BlacksmithBlessing more now does not have

I have the same problem
I can't use the blacksmith blessing

Merge branch 'master' into feature/refineui
# Conflicts:
#	conf/battle/feature.conf
#	src/map/atcommand.cpp
#	src/map/battle.cpp
#	src/map/battle.hpp
#	src/map/clif.cpp
#	src/map/clif_packetdb.hpp
#	src/map/script.cpp
@rAthenaAPI

This comment has been minimized.

Copy link
Contributor

rAthenaAPI commented Mar 23, 2019

This pull request introduces 4 alerts when merging 5345a0e into 73758ef - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same

Comment posted by LGTM.com

Merge branch 'master' into feature/refineui
# Conflicts:
#	src/map/battle.cpp
@rAthenaAPI

This comment has been minimized.

Copy link
Contributor

rAthenaAPI commented Mar 26, 2019

This pull request introduces 4 alerts when merging 01e45e6 into fded1aa - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same

Comment posted by LGTM.com

@jenkijo

This comment has been minimized.

Copy link
Contributor

jenkijo commented Mar 31, 2019

any news on this PR?

// Delete the item if it is breakable
if( materials[i].cost.breakable ){
clif_refine( fd, 1, index, item->refine );
pc_delitem( sd, index, 1, 0, 0, LOG_TYPE_CONSUME );

This comment has been minimized.

Copy link
@mrjnumber1

mrjnumber1 Apr 7, 2019

most likely this delete should match DELETE_TYPE_FAIL_REFINING. from aegis:

enum additem_result {
	ADDITEM_SUCCESS,
	ADDITEM_INVALID,
	ADDITEM_OVERWEIGHT,
	ADDITEM_RECEIVE_ITEM,
	ADDITEM_OVERITEM,
	ADDITEM_OVERAMOUNT,
	ADDITEM_REFUSED_TIME,
	ADDITEM_STACKLIMIT = 7
};
enum deleteitem_result
{
	DELETE_TYPE_NORMAL = 0,
	DELETE_TYPE_USE_SKILL,
	DELETE_TYPE_FAIL_REFINING,
	DELETE_TYPE_CHANGE_MATERIAL,
	DELETE_TYPE_MOVETO_STORE,
	DELETE_TYPE_MOVETO_CART,
	DELETE_TYPE_SELL_ITEM,
	DELETE_TYPE_EL_ANALYSIS,
};
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.