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

allow local NPC functions to be public or private #2142

Merged
merged 8 commits into from
May 27, 2020

Conversation

Helianthella
Copy link
Member

@Helianthella Helianthella commented Jul 23, 2018

This is my proposal to fix #2137:

  • local functions can no longer be called as events, even if their label begins with On
  • local functions may be explicitly exported by defining them as public function ....
    this allows other scripts to call them as if they were part of their own script
  • exported local functions can be called from any script by doing "npc name"::MyFunction();

-	script	npc1	FAKE_NPC,{

	private function NonExported {
		return "[1] non-exported";
	}

	public function Exported {
		return "[1] exported";
	}

	// local call:
	debugmes(NonExported()); // => [1] non-exported
	debugmes(Exported()); // => [1] exported

	// call in another NPC:
	debugmes("npc2"::NonExported()); // ERROR: not exported
	debugmes("npc2"::Exported()); // => [2] exported
}


-	script	npc2	FAKE_NPC,{

	function NonExported {
		return "[2] non-exported";
	}

	public function Exported {
		return "[2] exported";
	}

	// local call:
	debugmes(NonExported()); // => [2] non-exported
	debugmes(Exported()); // => [2] exported

	// call in another NPC:
	debugmes("npc1"::NonExported()); // ERROR: not exported
	debugmes("npc1"::Exported()); // => [1] exported
}
// local call syntax (unchanged):
<function>({<arg>...})

// public call syntax:
"<npc name>"::<function>({<arg>...})

// also:
callfunctionofnpc("<npc name>", "<function>"{, <arg>...})

@Helianthella Helianthella added type:bug Issue is a bug or describes an incorrect behavior that should be fixed type:enhancement Issue describes an enhancement or feature that should be implemented component:core Affecting the Hercules core (i.e. not the game mechanics directly) component:core:scriptengine Affecting the script engine or the script commands labels Jul 23, 2018
@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

src/map/script.c Outdated Show resolved Hide resolved
4144
4144 previously requested changes Jul 23, 2018
src/map/npc.h Outdated Show resolved Hide resolved
src/map/npc.c Outdated Show resolved Hide resolved
@4144
Copy link
Contributor

4144 commented Jul 23, 2018

and i think need add some tests to call local functions in CI

@Helianthella
Copy link
Member Author

@4144 yes, sure, I will do this after I write the docs

doc/script_commands.txt Outdated Show resolved Hide resolved
doc/script_commands.txt Outdated Show resolved Hide resolved
@Helianthella Helianthella changed the title WIP: allow to export local functions, and disallow calling them as npc events allow to export local functions, and disallow calling them as npc events Jul 25, 2018
@Helianthella Helianthella added this to the Release v2018.08.26 milestone Jul 25, 2018
@Helianthella
Copy link
Member Author

@4144 I have added unit tests

@Helianthella
Copy link
Member Author

Helianthella commented Aug 26, 2018

moving this to the next milestone

I felt the : prefix was ugly, so I will change the syntax to

function MyFunction { … } // implicitly private
private function MyFunction { … } // explicitly private
public function MyFunction { … } // explicitly public

It's cleaner, and the public and private keywords could also be reused in the future for other things

@Helianthella Helianthella changed the title allow to export local functions, and disallow calling them as npc events WIP: allow to export local functions, and disallow calling them as npc events Oct 22, 2018
@MishimaHaruna MishimaHaruna removed this from the Release v2018.11.18 milestone Nov 14, 2018
@Helianthella Helianthella linked an issue Apr 28, 2020 that may be closed by this pull request
@Helianthella Helianthella force-pushed the export2 branch 2 times, most recently from ba66d8c to 3e3c51d Compare April 29, 2020 22:30
@Helianthella Helianthella changed the title WIP: allow to export local functions, and disallow calling them as npc events allow to export local functions, and disallow calling them as npc events Apr 29, 2020
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 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
@Helianthella
Copy link
Member Author

I'll do some commit squashing once the review process is over

@Helianthella
Copy link
Member Author

I added two config flags for more flexibility:

  • functions_private_by_default defines whether functions are private or public when not explicitly set as public or private.
  • functions_as_events allows functions to be called as events if made public. This allows for backward compatibility if someone was using this bug as a feature.
-	script	@mycommand	FAKE_NPC,{
	public function OnInit {
		bindatcmd("mycommand", "@mycommand::OnCall");
		return;
	}

	private function doSomething {
		dispbottom(getarg(0));
		return;
	}

	public function OnCall {
		doSomething(.@atcmd_parameters$[0]);
		return;
	}
}

src/map/script.c Show resolved Hide resolved
src/map/script.c Show resolved Hide resolved
src/map/map.h Show resolved Hide resolved
@Kenpachi2k13
Copy link
Member

Furthermore your commits need some squashing. ☺️

@Helianthella
Copy link
Member Author

Helianthella commented May 2, 2020

@Kenpachi2k13 as noted earlier, I do intend on squashing once the review process is done.
It makes it easier for maintainers to review incremental changes when the branch is not force-pushed

@Helianthella Helianthella added the status:needs-squashing Extraneous commits should be squashed label May 2, 2020
@Helianthella Helianthella removed the status:needs-squashing Extraneous commits should be squashed label May 3, 2020
@Helianthella Helianthella changed the title allow to export local functions, and disallow calling them as npc events allow local NPC functions to be public or private May 3, 2020
@MishimaHaruna MishimaHaruna merged commit fcffb67 into HerculesWS:master May 27, 2020
@Helianthella Helianthella deleted the export2 branch May 27, 2020 12:50
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:core Affecting the Hercules core (i.e. not the game mechanics directly) type:bug Issue is a bug or describes an incorrect behavior that should be fixed type:enhancement Issue describes an enhancement or feature that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

local functions can be called as events
5 participants