Pryaxis / TShock

☕️⚡️TShock provides Terraria servers with server-side characters, anti-cheat, and community management tools.
GNU General Public License v3.0
2.43k stars 382 forks source link

Better command API #1736

Open AxeelAnder opened 4 years ago

AxeelAnder commented 4 years ago

Current command parameters processing is annoying. We really need a better command api like what the orion command issue describes.

I noticed there's a project which implemented similiar stuff. It'll save some energy if we integrate it into tshock.

hakusaro commented 4 years ago

@ZakFahey does the idea of rolling this into TShock core sound nice to you? I notice your project is MIT licensed anyway, but do you have opinions?

ZakFahey commented 4 years ago

Absolutely, go ahead. Next weekend actually I was going to update the core library this to fix some stuff, so maybe we can collaborate on that and hold off until I do that.

I've been using the library for a while on my server and there are some usability quirks I'd like to address, such as the fact that commands can't be asynchronous which is a pain if they include DB or RPC calls.

ZakFahey commented 4 years ago

By the way, I'm working on a new release of the base EasyCommands library and should have a release out by the end of today. It will add things like async commands, less bugged access control for subcommands, and default subcommands, among other things.

ZakFahey commented 4 years ago

What's the timeline you expect for integrating the plugin? Is this something you want to do before the full release of the 1.4-compatible version of TShock?

AxeelAnder commented 4 years ago

imo this will not involve with the 1.4 updating works, we can open a draft pr and begin now, when stable release out we can merge into main branch

ZakFahey commented 4 years ago

Yeah I figure. Just deciding whether to obsolete the easycommands tshock repo now or make a version tick upgrade to it. But yeah, you'd basically just want to replace out the commands system you have with EasyCommands while maintaining backwards compatibility with the old system.

ZakFahey commented 4 years ago

Oh apparently somebody else already has this in progress: https://github.com/Pryaxis/TShock/pull/1679. Did that venture not pan out for some reason?

Edit: well it does look like it hasn't been updated since October of last year.

kevzhao2 commented 4 years ago

So the venture sort of fizzled out due to a lack of general support on Orion being built. It's still something that I'm interested in doing...

A lot of the stuff built there is still usable, for sure.

ZakFahey commented 4 years ago

Call me biased, but I think it would generally be better to use the EasyCommands library rather than have TShock implement its own thing. It means that there's less code for the TShock team to maintain.

kevzhao2 commented 4 years ago

Yeah, definitely.

The original plan for that venture was to dynamically generate the most optimal parsing IL for a given command (to avoid reflection costs, support non-reflectable types, etc.), but with that kind of complexity, it's better to have a separate library for that.

ZakFahey commented 4 years ago

EasyCommands is reflection-based, but converting it to a codegen based system could be an option down the road. That being said, I've used the library on my own servers and I haven't had any performance issues with it.

ZakFahey commented 3 years ago

I see that this issue isn't particularly active, but I just want to put out there that if this is ever taken up, please for the sake of backwards compatibility do not remove the old command system. A new EasyCommand system could be there alongside the old version and we could mark the old system as deprecated, sure. But there are countless plugins that would have to be completely rewritten if the old command system is removed. I have plugins with dozens of different commands that I'd have to port over.

Just something to consider.

ivanbiljan commented 3 years ago

The idea is to keep the legacy system around at least for the initial release before getting rid of it completely (i.e until devs get comfortable with the new API). Mainly to facilitate the TShock4->TShock5 transition in case of community maintained plugins. That said, due to API differences the majority of plugins is likely to break regardless of the new command system.

bartico6 commented 3 years ago

if this is planned for orion/tshock5 you don't have to keep anything around from legacy because pretty much everything will have to be rewritten anyway