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

Add *duplicate/remove npc script command #2473

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Emistry
Copy link
Member

@Emistry Emistry commented May 14, 2019

Pull Request Prelude

Changes Proposed

  • npc_duplicate(....) duplicate any existing npc.
  • npc_duplicate_remove(...) will remove any existing NPC other than itself.

Recently kRO seem like added an item that will create a Treasure Chest NPC nearby player to retrieve item.

  1. click on the item
    consume item

  2. create a npc, talk to it, select to consume 1 key or 10 keys.
    create npc

  3. obtain item
    obtain item

Anyway, its a useful script command.
Original src : https://github.com/dastgirp/HPM-Plugins/blob/master/src/plugins/npc-duplicate.c

Issues addressed:
none

Known Issue:
dastgirp/HPM-Plugins#40
Not sure if this issue still persists, cant seem to duplicate this issue for now.

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@Emistry Emistry added component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder labels May 14, 2019
@Emistry Emistry mentioned this pull request May 14, 2019
3 tasks
doc/script_commands.txt Outdated Show resolved Hide resolved
doc/script_commands.txt Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
doc/script_commands.txt Outdated Show resolved Hide resolved
@dastgirp
Copy link
Member

Are those treasure chest seen by all players or players who used the item?

@Emistry
Copy link
Member Author

Emistry commented May 15, 2019

Are those treasure chest seen by all players or players who used the item?

No idea, I am curious too but I didnt found any info regarding this.

@Asheraf
Copy link
Contributor

Asheraf commented May 15, 2019

@dastgirp @Emistry it's visible to the player only, gravity uses the flag OPTION_CLOAK and sends it to everyone but the player.

@Emistry Emistry force-pushed the scriptcommand_duplicatenpc branch from e5c4f27 to 3164dc7 Compare May 16, 2019 14:47
@Emistry
Copy link
Member Author

Emistry commented May 16, 2019

I have no idea how to do the OPTION_CLOAK trick, maybe I shalll leave it to any of you who familiar to do it in future or push any new commit to this branch.

UPDATE:
seem like can be done using this ancient script command classchange(...).
Example:
when create the NPC, give a default sprite ID FAKE_NPC, then trigger this script command

classchange(4_TREASURE_BOX, 0, getcharid(0));

it will change the sprite into 4_TREASURE_BOX and only shown for attached player.
Hence, we don't need a new script command for it, nor do any adjustment to current PR since this is already implemented.

@Emistry Emistry force-pushed the scriptcommand_duplicatenpc branch 2 times, most recently from 96ecacb to 049300c Compare May 17, 2019 16:12
@dastgirp
Copy link
Member

dastgirp commented Oct 5, 2019

@Asheraf

Copy link
Member

@Kenpachi2k13 Kenpachi2k13 left a comment

Choose a reason for hiding this comment

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

This was my first source edit back then in 2012/13. Nice to see it again. 😊
Well, there are many things which should be improved...

src/map/script.c Show resolved Hide resolved
src/map/script.c Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Show resolved Hide resolved
doc/script_commands.txt Show resolved Hide resolved
@Emistry Emistry force-pushed the scriptcommand_duplicatenpc branch 3 times, most recently from 9401b3d to b1188ac Compare May 11, 2020 01:10
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
@Emistry Emistry force-pushed the scriptcommand_duplicatenpc branch 2 times, most recently from 88f0c3d to c42a1bc Compare July 5, 2020 18:39
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Outdated Show resolved Hide resolved
src/map/script.c Show resolved Hide resolved
@Emistry Emistry force-pushed the scriptcommand_duplicatenpc branch from c42a1bc to fc94cc0 Compare July 19, 2020 07:08
@4144
Copy link
Contributor

4144 commented Jul 26, 2020

ci still show overflow error

@Emistry
Copy link
Member Author

Emistry commented Aug 24, 2020

@4144
is there any advise for me to do it? the validation for npc name length has been done prior to this.
append targetname with \0 ?

@4144
Copy link
Contributor

4144 commented Aug 24, 2020

@4144
is there any advise for me to do it? the validation for npc name length has been done prior to this.
append targetname with \0 ?

look like compiler see issue in this code script.c:13085:3: note: length computed here 13085 | strncat(targetname, dup_hidden_name, strlen(dup_hidden_name)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

try change char targetname[NAME_LENGTH] = ""; into char targetname[NAME_LENGTH * 2 + 2] = ""; and here it should probably works without warnings

@4144
Copy link
Contributor

4144 commented Aug 24, 2020

also change strlen(dup_hidden_name) into NAME_LENGTH

- `npc_duplicate()` duplicate any existing duplicated NPC.
- `npc_duplicate_remove()` will remove any existing NPC other than source NPC.
@Kenpachi2k13
Copy link
Member

@Emistry
If you agree I'd take over this task...

@AnnieRuru
Copy link
Contributor

@Emistry
If you agree I'd take over this task...

101% agree + support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants