Skip to content

Conversation

@Emistry
Copy link
Member

@Emistry Emistry commented Apr 10, 2019

Pull Request Prelude

Changes Proposed

Check for unit data (nd->ud != NULL) before adjust the NPC speed.
Throw warnings if nd->ud == NULL.

Issues addressed:
script command npcspeed, npcwalkto, npcstop, unitwalk, unitwarp, unitstop are not applicable for floating npc (without coordinate/location).
Fix map-crash due to unit_data NULL.

Sample NPC Script:

-	script	dummynpc	-1,{
	OnInit:
		debugmes "set speed to 100";
		npcspeed 100;
		debugmes "set speed done";
		end;
}

Result:
sample

After fixed: throw warnings but didnt crash map-server
sample2

Related PR #1733

@Emistry Emistry added the component:core:scriptengine Affecting the script engine or the script commands label Apr 10, 2019
@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

Copy link
Contributor

@AnnieRuru AnnieRuru left a comment

Choose a reason for hiding this comment

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

there are similar lines in *npcwalkto and *npcstop
however when I test it, it doesn't crash server
so I guess its OK ... I guess ?

@4144
Copy link
Contributor

4144 commented Apr 10, 2019

@AnnieRuru this code may not crash server but corrupt memory

probably better fix it in all functions

@AnnieRuru
Copy link
Contributor

at least it doesn't cause memory leak ...
well, if just add in the same lines in those 2 script commands, simple I think

@Emistry
Copy link
Member Author

Emistry commented Apr 11, 2019

the reason it doesn't crash are probably due to :
npcwalkto() actually have the checking if( ud == NULL) return 0; added inside unit_walktoxy()
and
npcstop() actually have the checking if(!ud ..... ) return 0; added inside unit_stop_walking()

@AnnieRuru
Copy link
Contributor

how about just adding

		if (nd->ud == NULL) {
			ShowWarning("buildin_npcwalkto: floating NPC don't have unit data.\n");
			return false;
		}

instead of let the script command fail silently ?

- script command `npcspeed`, `npcwalkto`, `npcstop`, `unitwalk`, `unitwarp`, `unitstop` are not applicable for floating npc (without
coordinate/location).
- Fix map-crash due to unit_data `NULL`.
@Emistry Emistry force-pushed the scriptcommand_npcspeed branch from 563485a to 1259ea8 Compare April 11, 2019 15:34
@Emistry Emistry changed the base branch from stable to master April 28, 2019 17:01
@hemagx
Copy link
Contributor

hemagx commented Apr 28, 2019

Emistry, can you separate syntax fixing from the crash fixing? it's sort of impossible to review as it is right now.
separating each in it's respective commit would be a lot better.

@Emistry
Copy link
Member Author

Emistry commented Apr 29, 2019

huh? like how? reset then redo to create 2 different commits?
aren't at the end it still gonna end up become 1 commit that fix the same thing?
beside its not a huge commit to begin with, most of it just the if (ud == NULL) checking.

@MishimaHaruna MishimaHaruna added this to the Release v2019.06.02 milestone Jun 1, 2019
@MishimaHaruna MishimaHaruna merged commit 0e18ff7 into HerculesWS:master Jun 1, 2019
@Emistry Emistry deleted the scriptcommand_npcspeed branch June 1, 2019 15:17
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants