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

Proposal: Extract packets and events into their own library #2

Open
johnbanq opened this issue Aug 29, 2018 · 18 comments
Open

Proposal: Extract packets and events into their own library #2

johnbanq opened this issue Aug 29, 2018 · 18 comments

Comments

@johnbanq
Copy link

We now have like 10 mods in this repo,and stuff like packet sending & handling,player & world events are just basically lying around in modules that uses them,so why not aggregate them into a single library?
like chai_packets & chai_events.

This allows us to add things easier and know where to look for what we need

Pros

  • more organized codebase, easier to find API
  • aggregate places that "breaks" after MC updates

Cons

  • breaks existing plugins <- reason I am posting this as a issue instead of working on a fork then PR
  • will need ways to compile a module with dependency to other modules

PS: if this got accepted,what about packing modules into namespaces on the chaiscript side?
from onPlayerJoined to events.player.onJoined
from sendTextPacket to packets.sendTextPacket (or just packets.send with a overload)

PSS: This is more like a asking for permission than asking for change,because I thought I should ask before working on changes that breaks the API :)

@johnbanq
Copy link
Author

@codehz Hello? Is this an acceptable refactor? I dunno is there any plugin using them and how many of them will this break

@codehz
Copy link
Owner

codehz commented Aug 30, 2018

Well, chaiscript limit the type must be registered before registering the function, so if I aggregate them into one library, it may be required to load all mods every time..
And the namespace isn't as same as you expected... There many limitations applied to namespace, such as it doesn't support type registering, and overloaded function...
I separated the file depends on its dependencies

@johnbanq
Copy link
Author

johnbanq commented Aug 30, 2018

A quick view through the modloader's code shows that the problem of loading a mod multiple times does not exists,every .so gets loaded only once.
I am simply considering putting event handlers together to form a single library,IMO it shouldnt cause much performance issue

PS: It might surprise you but the libraries doesnt seems to work with the rewritten version of mcpelauncher,which launcher pairs with this library?

@codehz
Copy link
Owner

codehz commented Aug 30, 2018

@johnbanq I means if you only want to use chai_packets , but it reference a class that registered in chai_events so they all need to be added to the mods/ folder
or you can put all class registered in a single mod , I don't think it is a good solution... In fact, I want to link all the chaiscript related mod to be a large mod. :)

@johnbanq
Copy link
Author

oh,then the need of extraction is gone,but can we at least pack event handler functions into namespaces?
I am still trying to find a way to do so.
Can we pack all functions into namespaces,leave all classes defined in global,and expose some related classes into their respective namespace?

@johnbanq
Copy link
Author

Eg: onPlayerJoined -> event.onPlayerJoined
All Minecraft classes are left in the global
IntParamDef are defined in global scope and also aliased in cmd namespace,like cmd.IntParamDef

@codehz
Copy link
Owner

codehz commented Aug 30, 2018

@johnbanq TIP: namespace does not support function overloadding.

@johnbanq
Copy link
Author

but do we need them now? Any instance we have in the current version would require overloading?

@johnbanq
Copy link
Author

maybe we can get around with this on the C++ side by dispatches?

@codehz
Copy link
Owner

codehz commented Aug 30, 2018

TIP2: Namespace does not support nested

@codehz
Copy link
Owner

codehz commented Aug 30, 2018

I think using global object as namespace is a better idea(

@codehz
Copy link
Owner

codehz commented Aug 30, 2018

TIP3: Namespace cannot be extended in future, you can only define the entries at once

@johnbanq
Copy link
Author

um... using global objects? well,I guess I will have to give up the idea of organizing functions by namespaces

@johnbanq
Copy link
Author

does global object supports nesting?

@codehz
Copy link
Owner

codehz commented Aug 30, 2018

struct Events {
};
Events e;
chai.add(user_type<Events>(), "Events");
chai.add_global(var(e), "events");
chai.add(fun([](Events e, function<void(Actor)> actor) { ... }), "onActorDied"); 

produce
events.onActorDied(fun(Actor actor){})

@codehz
Copy link
Owner

codehz commented Aug 30, 2018

struct PlayerEvents {};
struct Events {PlayerEvents playerEvents;};
Events e;
chai.add(user_type<Events>(), "Events");
chai.add(user_type<PlayerEvents>(), "PlayerEvents");
chai.add_global(var(e), "events");
chai.add(fun(&Events::playerEvents), "player"); 
chai.add(fun([](PlayerEvents e, function<void(Player)> player) { ... }), "onDied");
events.player.onDied(fun(Player player){})

I think it is the right way to play with chaiscript's type system

@codehz
Copy link
Owner

codehz commented Aug 30, 2018

And I will not change the c++ source structure, but may be use the new style( global variable as namespace)

@johnbanq
Copy link
Author

johnbanq commented Aug 30, 2018

I guess we cant modify the source strucure without making the entire library monolithic(is it possible to adjust the organization of the codebase after merging?)

will go with the global object style,thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants