CivClassic / NameLayer

Allows the creation of configurable player made groups in Minecraft. Built for Paper 1.16.5
BSD 3-Clause "New" or "Revised" License
1 stars 19 forks source link

Make NameLayer compile without obscure/overcomplicated civ project setup process #37

Closed caucow closed 3 years ago

caucow commented 3 years ago

Per Discord:

legit if it doesn't compile immediately I'm going to break it and PR it

ProgrammerDan commented 3 years ago

there is a lot of anger in this commit

caucow commented 3 years ago

Yes and no, PR message is part of a longer convo reflecting on how awful civ plugin dev can be, especially just to set up a workspace for it.

Shadowtrot — 03/25/2021 [why is github so [bad at] such a simple feature [as searching for a full string] I don't want to have to download ja] am looking for the calling chain to see when it runs and have already given in and started cloning civ repos _DietCola — 03/25/2021 lol, cya in 8 hours pony :glad~4: Shadowtrot — 03/25/2021 yeah legit if [it] doesn't compile immediately I'm going to break [it] and PR it

Commit/deprecation messages on the other hand 100%: spaces > tabs (yes personal preference grudge ik); direct NMS references bad (objectively).

ProgrammerDan commented 3 years ago

space / tabs is a waste of arguing, just use whatever the project defines and live happy / free ;) (advice from the beyond)

The project was previously done like you've done here, and moved to NMS for clarity. Maven profiles are used to selectively build only the NMS profile necessary; this is easy to use.

I don't blame you for moving back to Reflection but it obscures the actual call used and tends to be more annoying to remember to fix later (as compiler won't complain).

I'd argue against it being objectively better, just different. Definitely less painful for new contributors, although the only pain in NMS is just, compile spigot / paper locally (which you should probably do when getting started).

There were a number of aborted attempts to shortcut this for new devs by "pre-starting" the process, one example was https://github.com/DevotedMC/devoted-vm which was a follow-on from the original civcraft-vm of the same purpose.

Regrettably shifting to non-NMS won't make any of this objectively easier, as end of the day you'll need to decompile Minecraft to figure out which NMS method you need to call via Reflection. It's just moving the pain point, and obscuring the pain a bit...

My two cents. I don't manage this repo so much love, thanks for being involved!

caucow commented 3 years ago

Yeah, ik the spaces argument is a waste of time (spaces are still better!!) and I won't go out of my way to change or ignore the project standard (still better) but that still won't stop me from giving someone hecc for it (because they're better).

As far as reflection obscuring calls, while I don't entirely disagree (one reason I use the overly-verbose-hard-to-read naming scheme I do for reflective accessors (ex. m_CraftHumanEntity_getHandle), because I know even I'm not going to remember what the hell each is for), I use it because in practice as a way of accessing NMS (ie not including programmer error/bugs/etc.), it's less maintenance in the long run as for the most part Paper obfuscation mappings don't change that much, generally leading to less need to rewrite parts of the plugin (as simple as they may be) as a requirement for using a different version of Minecraft or, in the case of a few individuals I've seen that have tried to use civ plugins on non-civ servers, allowing them greater flexibility on the Minecraft versions they use without having to learn how to go through said obscure/overcomplicated civ dev project setup and "code-it-yourself" or figure out which specific version of any given NMS-using plugin is compatible with their target game version (and potentially which dependent plugin versions are compatible with that plugin version), adding to the already tedious prospect of just setting up a civplugin compatible server. Ex. of this was that DevHell was stuck on early 1.16 versions for a while because the plugin compatibility wasn't there for 1.16.4/5 and updating NMS plugins takes time that could be spent "just" dropping in a new paper jar and working on other actual issues or new features, instead of cloning the latest paper repo and waiting for it to compile just to update a few compiler-bound references to NMS code. Also, while it is partially true that "shifting to non-NMS won't make any of this objectively easier, as end of the day you'll need to decompile Minecraft to figure out which NMS method you need to call via Reflection," it is still only partially true and on the whole is not: yes that will be a requirement if you want to write or work on NMS code, but it's less of a requirement than needing to do it (and then wait for the server to compile) for every single game version whether you want to work on said NMS code or not.

""TL;DR"" - yes, it's still a pain point, and honestly I partially blame Mojang for that, but beyond just "moving the pain point and obscuring it a but", it's also limiting who will need to endure that pain to "anyone that wants to work on the NMS bits" instead of "anyone and everyone that touches the repo". The point of plugins is that they're supposed to be something you can just drop into a server and it """just work""" after initial configuration, and likewise the point of having a cloneable repo to start dev work from is that you clone it and it """just works""" without having to already have an in-depth knowledge of the [civ] ecosystem (beyond just "it's a bukkit plugin") or the connections to other [civ] devs, ex. Orinnari who has the "just clone this and run this script" civdev setup repo, which is also cumbersome to someone who just wants to use or work on one plugin and not wait for the entire civ suite to download and [maybe or maybe not] compile.

ProgrammerDan commented 3 years ago

You make some excellent points, although I'd strongly disagree that the existence of a Git repo to clone is any kind of indication it should be easy to use. That's more of a modern dev-culture thing then anything, and while noble, has rarely if ever been entirely true in my experience.

Only other counterpoint I'd make, and perhaps I misread you so please ignore if I did, is that the NMS portions in NameLayer have not historically been remapped by Spigot, so they were very unstable, and the method signature changed every major release and protocol-R subrelease. Hence my perspective that this approach is a greater liability since it obscures this. If paper has moved the profile bits into their managed remap, then that objection is significantly reduced!

Thanks for taking the time to share your perspective!

On Wed, Mar 31, 2021, 20:58 Shadowtrot @.***> wrote:

Yeah, ik the spaces argument is a waste of time (spaces are still better!!) and I won't go out of my way to change or ignore the project standard (still better) but that still won't stop me from giving someone hecc for it (because they're better).

As far as reflection obscuring calls, while I don't entirely disagree (one reason I use the overly-verbose-hard-to-read naming scheme I do for reflective accessors (ex. m_CraftHumanEntity_getHandle), because I know even I'm not going to remember what the hell each is for), I use it because in practice as a way of accessing NMS (ie not including programmer error/bugs/etc.), it's less maintenance in the long run as for the most part Paper obfuscation mappings don't change that much, generally leading to less need to rewrite parts of the plugin (as simple as they may be) as a requirement for using a different version of Minecraft or, in the case of a few individuals I've seen that have tried to use civ plugins on non-civ servers, allowing them greater flexibility on the Minecraft versions they use without having to learn how to go through said obscure/overcomplicated civ dev project setup and "code-it-yourself" or figure out which specific version of any given NMS-using plugin is compatible with their target game version (and potentially which dependent plugin versions are compatible with that plugin version), adding to the already tedious prospect of just setting up a civplugin compatible server. Ex. of this was that DevHell was stuck on early 1.16 versions for a while because the plugin compatibility wasn't there for 1.16.4/5 and updating NMS plugins takes time that could be spent "just" dropping in a new paper jar and working on other actual issues or new features, instead of cloning the latest paper repo and waiting for it to compile just to update a few compiler-bound references to NMS code. Also, while it is partially true that "shifting to non-NMS won't make any of this objectively easier, as end of the day you'll need to decompile Minecraft to figure out which NMS method you need to call via Reflection," it is still only partially true and on the whole is not: yes that will be a requirement if you want to write or work on NMS code, but it's less of a requirement than needing to do it (and then wait for the server to compile) for every single game version whether you want to work on said NMS code or not.

""TL;DR"" - yes, it's still a pain point, and honestly I partially blame Mojang for that, but beyond just "moving the pain point and obscuring it a but", it's also limiting who will need to endure that pain to "anyone that wants to work on the NMS bits" instead of "anyone and everyone that touches the repo". The point of plugins is that they're supposed to be something you can just drop into a server and it """just work""" after initial configuration, and likewise the point of having a cloneable repo to start dev work from is that you clone it and it """just works""" without having to already have an in-depth knowledge of the [civ] ecosystem (beyond just "it's a bukkit plugin") or the connections to other [civ] devs, ex. Orinnari who has the "just clone this and run this script" civdev setup repo, which is also cumbersome to someone who just wants to use or work on one plugin and not wait for the entire civ suite to download and [maybe or maybe not] compile.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CivClassic/NameLayer/pull/37#issuecomment-811562742, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADD5DUUYZIMGDC5LQ7OES3TGPAMLANCNFSM4Z223Y7Q .

wingzero54 commented 3 years ago

Sorry but no