OnNPCKillEvent changes

[AnnieRuru 957]

https://github.com/HerculesWS/Hercules/pull/2061
before understanding this pull request, let's talk about the history of OnNPCKillEvent

History

during the time OnNPCKillEvent implement, monster only spawn on the fields and dungeon, in this syntax

** Create a permanent monster spawn:

<map name>,<x>,<y>,<xs>,<ys>%TAB%monster%TAB%<monster name>%TAB%<mob id>,<amount>,<delay1>,<delay2>

and the monsters that spawned with event labels were only use in job changer quests

*monster("<map name>", <x>, <y>, "<name to show>", <mob id>, <amount>{, "<event label>";}

Note: if you noticed some parameter missing, yup those were added later.
Notice the permanent monster spawn during that time still doesn't support event labels

So, in order to trigger the permanent monster spawn, OnNPCKillEvent was the only way (during that time)
this was to separate the trigger between OnNPCKillEvent and monster with event labels

Why separate them ?

there's a good reason to separate them, aleos also said in this issue
in fact, this bug was also brought up several times during eathena ...

Let's give 2 examples:

Example 1: Bot-killer script

Bot-killer script is intended to kill bots, and bots usually only appear on fields/dungeon
which makes OnNPCKillEvent: label an ideal solution to work on them

currently, Bot-Killer script doesn't trigger with job changer script or event script
because the job changer npc, the monster was spawned with event labels
example like priest job change quest -> you have to kill all the undeads within 5 minutes

imagine ... IF the Bot-Killer script was able to trigger monster with event labels,
while the players was rushing against time, suddenly a bot-killer script pops up !!
this is enough to make them fail the test ...

same thing goes to other event scripts such as devil square
when players were busy killing monsters, trying to get the most kills, suddenly a bot-killer script pops up !!
enough to make them lose the 1st place

Example 2: MVP ranking script + MVP Ladder game

MVP ranking script ... show the top 10 MVP hunters in your server
MVP ladder game ... a game to form a party then kill MVPs inside arena

currently, the MVP ranking script doesn't record the MVP kills from MVP ladder game
the reason is ... the MVP ladder game spawn the MVPs with event labels ...

imagine IF the OnNPCKillEvent label able to trigger monster with event label
players just has to replay the MVP ladder game again and again to earn themselves the best MVP hunter ...
each game adds 39 kills, so cheap !! Don't need to find MVPs on the field anymore
now that's defeat the purpose of MVP hunting .... I mean the MVP ranking script

So why propose the change now ?

Things has changed since then, especially with the introduction of instance script
since all instance monster has event labels, previously said that monster only spawn on the fields and dungeon no longer apply

Take a look back at Example 2 ... the MVP ranking script
currently the MVP ranking only record the MVP kills that spawn MVP tombs
but it doesn't record the kills from instance ... for example Nacht Sieger or Nidhoggur's Shadow or even Lighthalzen MVP

---

Now here's the tricky part ...

Example 1: Bot-Killer

If we keep it as it is, the script works fine
and if let OnNPCKillEvent run event labels, bot-killer can trigger inside job change npc (BUG)

Example 2: MVP ranking script + MVP Ladder game

If we keep it as it is, the MVP ranking script doesn't record the MVP kills from instance script (BUG)
and if allow OnNPCKillEvent run event labels, MVP ranker script record the kills from MVP ladder game (BUG)

both options are not a perfect solution

but there is a way to actually solve all these problem,
find this line

OnNPCKillEvent:

replace with ...

OnNPCKillEvent:
	if ( getmapflag( strcharinfo(PC_MAP), mf_nosave ) )
		end;

let ALL OnNPCKillEvent: doesn't trigger on the map that has nosave mapflag
simple because, all job changer npc and event maps has nosave mapflag ... this is easy
this simple solution actually solve both example's problem above

.... well actually I also has another patch ready ...
but not sure if this setting make things more complicated ?
well ... currently still in the discussion stage ~

Edited June 9, 2018 by AnnieRuru

[meko 170]

6 hours ago, AnnieRuru said:

but there is a way to actually solve all these problem,

find this line

OnNPCKillEvent:

replace with ...

OnNPCKillEvent:
	if ( getmapflag( strcharinfo(PC_MAP), mf_nosave ) )
		end;

 

 

if one doesn't want to rely on mapflags, they could also make the maps use a custom zone, and then it can be checked with

OnNPCKillEvent:
    if (getmapinfo(MAPINFO_ZONE, strcharinfo(PC_MAP)) == "my_zone")
        end;

 

[AnnieRuru 957]

3 hours ago, meko said:

if one doesn't want to rely on mapflags, they could also make the maps use a custom zone, and then it can be checked with

OnNPCKillEvent:
    if (getmapinfo(MAPINFO_ZONE, strcharinfo(PC_MAP)) == "my_zone")
        end;

 

this means they have to do their homework

well, remember ... most members download hercules emulator without fully understanding how the script works ...

[Emistry 145]

-1

using mapflag like that are just a trick ... it's not a solution to solve the issues.

i would have wish it was implemented with a different parameter in the *monster() for it to enable trigger the event or at least a custom mapflag or zone at least.

i believe most of us sometime would write script the way we wouldn't want it to trigger server wide...and yet this changes make it trigger server wide...unless it was using the mapflag tricks.

 

 

[AnnieRuru 957]

6 hours ago, Emistry said:

-1

-1 on the PR itself or just the nosave mapflag suggestion ?
I didn't actually put my patch into the PR yet, because I knew its cheap hack

 

6 hours ago, Emistry said:

i would have wish it was implemented with a different parameter in the *monster() for it to enable trigger the event or at least a custom mapflag or zone at least.

the point is ... triggering from job change quest monsters, event monsters and instance monster all sharing the same trait -> having an event label
and they are in the official scripts

how many lines in the official scripts you want to change if to enable monster to trigger for OnNPCKillEvent ?
how many maps needed if made a custom mapflag to run OnNPCKillEvent ?

the idea having loadevent mapflag to trigger OnPCLoadMapEvent should have drop long ago ... nowadays computer power is superior than before

 

6 hours ago, Emistry said:

i believe most of us sometime would write script the way we wouldn't want it to trigger server wide...and yet this changes make it trigger server wide...unless it was using the mapflag tricks.

com'on you also made mission board before ... you should understand when people say killing lighthalzen mvp doesn't trigger OnNPCKillEvent but normal mobs do

 

yes, like I said in the 1st post, it was already done that way to separate them, and it worked before long ago, but time change things

[Emistry 145]

I understand the need for it to trigger the OnNPCKillEvent for these event/quests.

3 hours ago, AnnieRuru said:

how many lines in the official scripts you want to change if to enable monster to trigger for OnNPCKillEvent ?
how many maps needed if made a custom mapflag to run OnNPCKillEvent ?

as many as we would need, and these efforts i believe it would be just the same if you compare with restructure the db to libconfig and others, just saying the effors

 

3 hours ago, AnnieRuru said:

nowadays computer power is superior than before

true, but it could be still performance killing. If this statement are acceptable, the old mob controller and any other OnPCXXXEvent would have no problem getting it implemented into the emulator in the first place.

 

[AnnieRuru 957]

12 hours ago, Emistry said:

If this statement are acceptable, the old mob controller and any other OnPCXXXEvent would have no problem getting it implemented into the emulator in the first place.

now I am agreeing with you
I already started making the mobevent script command ...
but I found our/hercules setunitdata are mostly broken so I have to fix that one 1st ...
that took me a lot of time since I have to test them case by case basis