CraftBukkit fixes and improvements that I lost hope for

Discussion in 'Bukkit Discussion' started by bergerkiller, Sep 18, 2012.

Thread Status:
Not open for further replies.
  1. Offline

    bergerkiller

    Here follows a list of CraftBukkit bugs, problems or general code hazards that I don't expect to be fixed. These include simple line changes or other. Their reasoning for not fixing it is: 'this is of Mojang, we should not care', yet it still harms the developers that try to push CraftBukkit to it's edge.

    I also list the changes I made on my side to add support for these bugs.

    TileEntity world field is not visible and there is no getter
    Source file link
    Due to it being invisible, it is not possible to work with Tile Entities as a parameter without adding an additional reflection field to get the world from it. Which I did.

    PlayerInteractEvent is no longer working when player is holding placeable blocks
    Something with 'relies on Client messages'. I don't get it, you can handle the block placement, and add an interact event, right? Can't do much about it myself.
    Ticket, but was closed for some reason

    Chunk [0, 0] is randomly unloaded when players move fast
    Probably a Mojang bug, but should still be fixed. See this page.
    I added changes in NoLagg's dynamic view to deal with this properly. The hackish way, that is.

    Vehicle create events are fired from within minecart/boat constructor
    Results in you receiving an entity that doesn't even exist on the world, which can cause all sorts of odd bugs.
    Source file link 1
    Source file link 2
    It should be moved to:
    World link

    It is not possible to teleport non-player entities between worlds
    This is my solution logic. Which also takes care of passengers.

    Location objects is causing memory clutter
    Code style: Whenever someone calls entity.getLocation or when events are fired, a new Location is created on the stack. Unfortunately, it uses 3 doubles, 2 floats and a World, and can take up a lot of memory. This makes heavy operations take 20x as long. I had to work with raw variables instead, because I couldn't call entity.getLocation() 10K times without lowering tick rate.

    World region files are not closed upon world unloading
    It makes it impossible to delete worlds after you unload them. The streams have to be closed to fix this. I fixed it in MyWorlds using this code.

    Important values are missing or inaccessible in entities
    For example: minecart fuel, arrow hit block position and creeper fuse ticks. For now, I solve this with about 20-30 class files full of reflection Field (wrapper) classes in BKCommonLib.

    World set type ID is slow - lighting is being performed
    There is no multi-block change feature in Bukkit. There is no way to start block updating, mass-update and then stop block updating, after which lighting is re-initialized. This makes a lot of 'huge block change' plugins impossible to make...without using CraftBukkit of course. There are ways around it if you don't use Bukkit.

    Redstone events are bugged
    Levers don't always send redstone updates when toggled through code, even though physics (should) take care of that at some point, right? I had to use the block physics events for 60% of the redstone updates in Redstone Mania and TrainCarts because of this issue. (including a powered blocks set, unfortunately)

    Entity add and remove events are missing
    Instead of the many 'despawn' and 'spawn' events, work on two simple events: add and remove. It covers everyone's needs. I had to add my own IWorldAccess (now WorldManager due to unsafe casting) to handle these events. It overcomplicates everything way too much.

    Are all these events really needed? For real? Isn't one event for this enough?

    You can replace the following events:
    • ItemDespawnEvent
    • ItemSpawnEvent
    • CreatureSpawnEvent
    • VehicleCreateEvent
    • ProjectileLaunchEvent
    • ExplosionPrimeEvent
    With:
    • Entity add event (both when chunks load and when spawning)
    • Entity remove event (both when chunks unload and when removing)
    And have more use cases as a result as well. Now you can listen for ANY entity, precisely keep track of your entities without using a tick task and a collection of entities and reduce classes in the process. This would rid me of using the WorldListener (nms) to handle it, and would greatly reduce lag as well, as I can then cancel the event instead of setting the entity dead. (if you could also allow changing the entity being added, TrainCarts would be a lot easier to maintain!)

    Chunks and worlds don't allow you to iterate over tile entities
    What's wrong about iterating over all the chests, signs and furnaces in the world or chunk? They are in CraftBukkit, and I use them a lot.

    Proper material<>name mapping doesn't exist
    Had to implement my own. A configurable name<>id file on the server would make the developers' lives a lot easier. Right now everyone creates their own massive if-statement lists...or a map of them, like I have in BKCommonLib.

    Impossible to control pistons and redstone programatically
    There is no way to make a given block 'glow up' automatically. Not redstone either. The only possibility is levers right now. I doubt this will ever be introduced...
    Pistons can not be controlled...at all. For some reason doors work, but pistons remain impossible to work with. I think a lot of people would be happy when pistons get a proper API.

    Entity spawns can not be cancelled before creating the entity
    It makes spawn limiting plugins impossible to work with. PreSpawnEvent perhaps? Anyhow, the internal spawn mechanism causes a lot of tick rate loss when you try to block spawns.

    Chunk unloads are spammed
    There is no 'prune' time for chunk unloads. When a chunk is not seen by a player, the server tries to unload it tick after tick. Why allow it to be cancelled when you cause it to kill the server? Really, when a plugin denies you to unload it, stop attempting to unload that chunk for the next 200 ticks or so?

    Mysterious world load and unload bugs when teleporting
    After unloading and then loading a world, it is impossible to teleport to it without completely bugging yourself. You basically become a split personality, you visual self being in one world, your interacting self on another. It makes 'temporary worlds' impossible to do, you can't reload worlds.

    No Bukkit offline Block storage
    There is no class, like Location or Vector, which stores the world name and the xyz of a block. This makes it impossible to store blocks safely offline by players that want to. Using Block objects causes a lot of problems when worlds unload, and it is not possible to store blocks of unloaded worlds as a result.

    Now before you hate on me...

    I just want to get all these problems 'out there' while possible. I stopped filing bug reports after three 'come on, why not? This benefits all' reports were marked invalid because they covered the nms namespace. Even if they are about Bukkit features that use that namespace to even exist.

    So feeling bored? Want to do something? Look at one of these many problems and see what you can do. Who knows, maybe they will accept your pull request.
     
    MyPictures likes this.
  2. Offline

    TnT

    Do you have PR's for these points? Even closed ones?
     
  3. Offline

    bergerkiller

    TnT
    Most of them don't even make it as ticket, so no PRs. And some I haven't reported either. This is just a general page to list them, so they don't get lost in the end.
    Let's just say that this ticket made me slightly give up on reporting bugs.

    I'm pretty sure that the server knows when someone right-clicks a block...block placement perhaps? I right-click (interact) with another block to place against.

    Invalid 'because the client doesn't send the messages' is just plain wrong. If that were true, you couldn't even place blocks.

    In closing, a question: Why are tickets invalidly marked invalid?
     
  4. Offline

    Wolvereness Bukkit Team Member

    So, you want something useless? Give us a reason to add it... API! Bukkit is an API, and CraftBukkit implements it. If it isn't part of Bukkit, we have no reason to change CraftBukkit.
    Packet system was changed. Go complain to mojang. The client 'normally' says that it is touching a block, but when it 'knows' it can't place the block, it tells the server "I'm touching the air". So, without ray-tracing (oh God!) we can't change this ourselves.
    Um... Seriously, why are you blaming us for this?
    PR and bug report...? Or does it actually cause any bugs?
    Considering how Minecraft works, I know you know nothing about the way that minecraft actually works. CraftBukkit player teleporting is specific to CraftBukkit, Minecraft actually deletes the old player. No other entity travels between worlds. Provide a use-case that can't be done with the correct way of removing an entity and recreating them.
    Lolwut? We provide defensive copies... Stupid crap happens if we were to give you the same location that we gave to someone else. Or would you rather us massively break every plugin everywhere by making Location immutable, degrading performance even more because we have to create new ones every time something changes? Please just save the reference to the object when you call getLocation(). Unless you do it 20 times per entity per tick per calculation, it is next to nothing to provide defensive copies.
    Bug report? Oh wait...
    Important is relative. I think there's only been one PR between the three of those.
    Of course, there's no PR for this one either, is there?
    Physics is per-block. Perhaps you should apply physics to both applicable blocks, or is that too much? If there is no API, provide a PR... (I'm seeing a pattern here)
    Covers needs? What needs?
    Again... Use-case and pr?
    It's called org.bukkit.Material...
    Should I stop asking for PRs?
    How can you cancel something before you even know what's going to happen? Perhaps with some type of API that describes the features of the entity about to be added.... Oh wait, there is something for that. It's an Entity!
    You need to be more descriptive about this, because it sounds very deceptive. The problem with chunks unloading after being cancelled is a very specific problem to certain circumstances. Until I see a PR to fix this (oh, there's that PR term again), I don't think you should be complaining.
    Bug report?
    Bukkit should not create a full blown database, you should use a plugin for that. There is no sane reason to add that information to blocks.

    Um... You make bug reports because you do something we don't support in NMS -> closing as invalid. What do you expect? Perhaps you should write pull requests with accompanying (but appropriate) feature requests instead of writing essays complaining about the lack of features.

    Your little soap box ("I just want to get all these problems 'out there' while possible") is worthless. Instead of actually examining the situation from a Bukkit/CraftBukkit perspective, you turn around trying to rally support by supplying a limited and bias view of problems.

    Here's a flowchart of how to help Bukkit: [​IMG]
     
    bergerkiller likes this.
  5. Offline

    bergerkiller

    Wolvereness
    In advance: I am not a CraftBukkit developer. I work *with* CraftBukkit, and these are all problems I got while working *with* it. I know how to fix it, but can't be arsed to download the entire CraftBukkit project, make changes, send a PR, and then seeing it being denied. So far 80% of even my tickets get denied, so you have to understand I don't have time for wasted PR's. So: Am I part of the CraftBukkit development team? No. Do I bother writing PR's that get ignored or closed like my tickets? No. Do I expect the 'real' development team to try and fix stuff? Yes. Does this happen? No, not in my experience at least.

    1.) Well that is a bug then, that the old entity is removed. It is not needed...at all. I can teleport players and other entities using my method without ANY problems. Why can't CraftBukkit do it? Why do entities simply disappear when teleporting to other worlds?

    2.) getLocation is the only way to get entity coordinates. There are no alternatives. If you want to do a global entity distance check, it is not possible to do because of the many Locations that are created. Add getX/Y/ZPosition at least, or allow you to fill a Location object by passing it into a function.

    3.) You can add an event before you create a new mob in the spawning mechanism in the world. Only the EntityType is needed for the event.

    4.) Material.byName is incredibly hardcoded. It has to match the exact Material name. For example, I can't send in 'golden apple', it has to be 'golden_apple'. This applies to all materials. There should be material name aliases configurated somewhere, but there aren't. It always has to be with the '_' in the name.

    5.) Player interact bug you mentioned seems fishy. I had to manually include a distance check to prevent blocks being placed inside the player. That interaction with air is sent when near a block is news to me.

    Say hello to BKCommonLib. It is big enough as it is.
     
  6. Offline

    Wolvereness Bukkit Team Member

    If you needed a 'title' to contribute, every open source project ever would have failed. That's the most ridiculous assertion I've ever heard. Again,
    Try figuring out why PRs or tickets get declined. Justify the change. Do it correctly, efficiently, and completely.
    1. I'm assuming you are referring to the entity teleporting thing. Try examining the code, we jump through a LOT of hoops to teleport players properly. Trying to 'switch' the world of an entity has consequences. Minecraft wasn't designed to do it, with a perfect example being players. Teleporting is a CraftBukkit concept.
    2. I haven't seen this suggested yet you expect us to have done it? Where's your PR? Oh wait, you don't want to help the Bukkit project because... Personal reasons?
    3. Again, that is self-defeating. What is so wrong with creating an entity object? Cancel the damn spawn event... An obsessive fascination with object pre-creation is not healthy. Show justification, a use-case.
    4. Material.matchMaterial(String)
    5. It might seem fishy, but try looking at the damn code before blaming us. Yes, if you desperately need to figure out if a player actually clicked on a block, you gotta ray-trace it. Do you actually want that occurring for every right-click of every player in the server? The client tells the server what it tells the server.
    If you think a common 'library' needs an integrated database system, then more power to you. That just gives developers choice in how to implement said system and how they want it to behave. It's just not appropriate to add into Bukkit.
     
  7. Offline

    bergerkiller

    Wolvereness



    I understand the reasoning better now, thanks. And reason I do not work on CraftBukkit is that I have to work with 6 (actually 7) projects already. Any time I have left would be put into Spout development, which needs it most. You have to understand that it annoys me greatly that I have to basically write an API for an API. It shouldn't be needed to write your own teleport, block setting, getting and world unload code 'just because' the one in the API doesn't work as expected. It is even more annoying if you know that your ticket that requests these things to be fixed, will get marked invalid, because it is about net.minecraft.server. Hence, the title: I do not expect them to be ever fixed.

    But ok, I'll just keep on adding my own implementations in BKCommonLib. It is an open-source library, so if someone wants to cut a bit out of it and put it in CraftBukkit, go ahead.
     
  8. Offline

    Ferkswe

    Took me like 20 minutes to figure out what PR was.
     
    Codisimus and thernztrom like this.
  9. Offline

    TnT

    I try to take this approach with everything in life: If I'm not willing to put in the time and effort to accomplish some task, how can I possibly expect someone else to do so? This is especially true when I have been given the ability to do so myself.

    I can understand being shy to work on something because your idea or PR may get rejected. However, that's life. Not every idea I have will be golden, nor will every piece of code I write be up to standards. This is why Bukkit/CraftBukkit is a team project - everyone has to discuss and agree on the idea/code submitted before it makes it into the codebase. This is not a cowboy project, it is a well structured, well maintained system.

    We do not do this to be a pain, but rather to ensure the project vision is followed and the code is sane. Without this measure, you'd see any number of plugin features added directly into CraftBukkit instead of handled exactly like they are now - via plugins.

    You give up so easily.
     
    Wolvereness likes this.
  10. Offline

    bergerkiller

    Which is usually a problem. It requires tonnes of support whenever a new version comes out, and fields change. Most non-obvious fixes have to use reflection because CraftBukkit doesn't expose a lot. That combined with constantly changing variable names, of course. If these features were in CraftBukkit directly, the field is automatically updated during the initial refactor.

    Also, it causes the many CraftBukkit forks like Spigot and CraftBukkit++ to fill the gaps. Why aren't these fixes in CraftBukkit ANYWAY? Some of the fixes are so simplistic and obvious it is almost a crime not to do it...

    (or is this still about Mojang prohibiting any fixes Mojang should make instead?)
     
    yoyo and The Mad Chem1st like this.
  11. Offline

    TnT

    Mojang doesn't limit what we do. Minecraft limits what we do. If there is a change that completely, knowingly, breaks vanilla behavior (such as you find in CB++), it will not get pulled. We're not here to make our own voxel powered server, we (more specifically, the team coding Bukkit/CraftBukkit and the community providing PR's) are making a better Minecraft server that can be extended via plugins. Spigot contains many Bukkit Bleeding branches that have yet to be tested, some of which are good and make it into CraftBukkit after adequate tests, while others do not because they are not proven to help. We get yelled at by GSP's, Server admins, Plugin devs, and anyone else when we break things, Spigot/CB++ have the luxury of not having that problem.


    See the many requests for PR's in this thread.
     
  12. Offline

    bergerkiller

    TnT
    I guess I can understand that, yes. Time to adjust my viewpoint of CraftBukkit I suppose, I expect too much from a project that is meant to be simplistic in use. And yes, I admit that I have a lot of things I would like to be fixed (and turned into a PR by someone), but some of them are probably more suitable for a library plugin like I made.

    Though still, there are a few PR requests (as you call them) that are more like a Bukkit compatibility update for Minecraft Server than something for a library. Entity add/remove events are a thing I never really grasped of being missing. Why have VehicleCreate, MobSpawn, ItemSpawn, ItemDespawn, VehicleDestroy and so many more if they can all be fitted in an Entity add and remove event? Wouldn't it be a lot easier (for Bukkit) if these events are merged?

    Might consider writing a ticket for it, but I fear I get the same 'we do not support nms'...sigh.

    Are there any guidelines for 'things the CraftBukkit team is willing to fix or implement'? Could very well be all they do right now is handle Bukkit-related bugs in the org.bukkit namespace...
     
  13. Offline

    Wolvereness Bukkit Team Member

    Hindsight... Changing it now is just breaking shit for the sake of breaking it. Even if the community sometimes disagrees, Bukkit takes great pride in not having plugins break over time, with the event handling being an exception. There was plenty of justification for it, but it's one of those things.
    If you ask us to fix a bug that can 'never' occur while using the API, we will refuse. Either show that it is caused by appropriate use of the API, or ask for new API that can accomplish what you want without forcing you to hook into NMS.
    If you want a feature, it must have API. If you are adding API, it needs a use-case. When you have a use case, you need to ask yourself "what is most appropriate for this use case." If you don't pop right back to the API you are suggesting, we will say no. An API needs to fix the use-case the proper way, from a "what should be the solution for this use case" instead of a "yes, this could allow for a workaround in this use case, but it doesn't fit." You need to think of the use case before thinking of the API, something many PRs fail to see resulting in developers being frustrated. But, from a maintainer perspective, we want 'good' API, not just an API bastard child of specific use cases married and hundreds of implementation styles. You outlined one of the very things we are actively trying to avoid; many different events that should've been designed differently and coherently.
     
    bergerkiller and TnT like this.
  14. Offline

    Synaps3

    I just want to chime in and thank Berger for making this thread. The content of it is above my ability, but he's trying to help us improve bukkit and I want to recognise that.

    I see him getting a pretty agressive dressing-down from Wolvereness and TnT, but I'm not sure it's deserved. He may not have time or interest or even be disillusioned with Bukkit updates, but he's trying to bring up things that were difficult for him as a dev, in a way that he hopes will let them be easier to manage for others than they were for him.

    We could all do well to remember that complaints are a way of showing that you care about the state and outcome of something. Berger wants to see Bukkit improved. Even if those improvements are midguided (most are beyond my level as a dev) it doesn't mean coming down hard on him is a good idea. Just because he isn't willing to go start making additions to bukkit himself doesn't mean we should denigrate his efforts to improve the code we all work with, either.

    Community projects like bukkit survive on interested people who want to see them made better. Despite being disillusioned with his results so far, Berger is one of those people who is trying to help the community make bukkit better. That spirit should be encouraged, not attacked.

    That's why I'm making a post to say thanks, Berger.

    2c.
     
  15. Offline

    TnT

    Synaps3
    You misunderstand our responses if you feel they are "aggressive dressing-downs".
     
  16. Offline

    Wolvereness Bukkit Team Member

    He threw around phrases like "not caring." That particular phrase feels like an assault when you use it to reference something I'm passionate about. As far as helping other developers, he's encouraging a very, very poor practice of using things in NMS to achieve functionality instead of pushing for appropriate API. We (as the Bukkit team) are unable to support the NMS namespace. It changes every patch, as well as being our methodology to keep the API working. When a patch comes that breaks what API we provide, we have to change things in the background. So many times we do this, some plugin author comes around complaining because they refused to work with the API and their plugin broke. The contract that we provide is that we will do what is needed (if possible) to maintain the API. When you work with the API, your plugin should not break between versions. PlayerChatEvent is a great example of this. The NMS implementation changed! The entire concept of chat changed, yet we maintained the old API and make it continue to function like it did before. This becomes a best effort basis. When you consider how often things change in NMS, there's no way a group of volunteers could maintain everything between versions.
    He is using this as a soap box. It's somewhat hypocritical to make the improvements in a private project of his that has his name on it (see: BK...lib) but turn around and say that he can't be bothered to contribute to Bukkit. What separates this post from one with almost the same content, but instead asks for community feedback and assistance in how to implement what needs to get implemented? Mentality. Getting angry and degrading a project (or person) isn't a proper response when you are frustrated (btw, this is a life lesson some people learn and some people don't).
     
  17. Offline

    bergerkiller

    Wolvereness
    I agree on your post, too. I understand that me linking to a personal (compatibility) project makes it look like I made changes but don't want to add them in Bukkit myself. Problem is, though, that all of my changes work with reflection to make it work. The 'Bukkit way' would be without reflection, and probably completely different, because then I can change the method, instead of turning a List or LinkedList into an event listener.

    You are right about me being frustrated. The 'Bukkit' experience has drastically become less over time. Right now I am basically programming net.minecraft.server, because Bukkit doesn't support half of the things I need. The only things I use right now are the events, block, world, chunk and player classes. All other variables I receive from events are instantly converted to a net.minecraft.server.Entity or similar. BKCommonLib is basically an API around net.minecraft.server. It is full of utility methods for native programming, mainly to do stuff that changes whenever a new CraftBukkit build comes out. For example, the 'b' field in the EntityTracker, which contains all the EntityTrackerEntry values for entities on a world. As I read from your post, it is not important for the API to make it public and rename it, so I end up using reflection to do it. There are a lot of examples here, of course.

    But now I know, at least. CraftBukkit is about Bukkit and doesn't care about the net.minecraft.server running behind it. My personal task as a nms developer is to write API functions for stuff Bukkit refuses to work with. Which are pretty fun things to work with, like entity replacements, things like mobdisguise, AI changes, massive block updating and tile entity changes, like changing what every player sees on signs separately.
     
  18. Offline

    deltahat

    bergerkiller
    Please pardon my wall of text.

    This whole back-and-forth seems a little ridiculous to me. In the open source world there is significant value and great precedent for the type of work you are doing with BKCommonLib. The common term for what you are making is called a contrib library, and there are lots of them for many kinds of projects. Typically, contrib libraries provide additional optional API for an existing project that the project's team considers outside of scope, dangerous, or experimental.

    As members of the bukkit team have previously addressed above, some parts of what you are doing lie outside bukkit's narrow scope of staying true to Vanilla Minecraft. Other parts of what you are doing makes substantive changes to Mojang's NMS code which the bukkit team has made a policy of not changing. Finally, some parts of what you have built have not been implemented internally yet because there is simply not enough man-hours available to implement proper API for all things while also maintaining a project as large as bukkit has become. While the bukkit developers have chosen to take a conservative stance concerning their code base, that does not degrade the value of the work you have done nor does it depreciate BKCommonLib.

    It is my belief that much of the friction in this tread stems from the differing priorities between you and the bukkit team. The bukkit team wants to maintain stability and backwards compatibility above all else. You are looking to push the minecraft code base as hard and far as is possible. This difference of priorities is exactly why community contrib librries exist and why, in my opinion, you should keep BKCommonLib separate from bukkit and seek to expand the functionality it provides rather than trying to fold all of your code into the main bukkit source.
     
  19. Offline

    chaseoes

    Why can't you give the guy a simple yes/no answer? I'm sure you can glance at the "fixes/improvements", and know right then if a pull request implementing them would be accepted or not.

    If it sounds like something that would be accepted - great! He can make PR's and the corresponding tickets.

    But if you already know "that will never be accepted", why waste his time and force him to make a PR and corresponding ticket that won't get accepted anyways? Just so it can be "officially" denied?
     
  20. Use cases:
    1) Teleport the entity with all potion effects, noDamageTicks, ... in short: With all metadata.
    2) Don't change the ID/UUID of the entity so other plugins are still able to track it if needed.
    Plugins which would benefit from it:
    http://dev.bukkit.org/server-mods/inception/
    http://dev.bukkit.org/server-mods/v10verlap/
    http://dev.bukkit.org/server-mods/voidwarp/
    http://dev.bukkit.org/server-mods/worldwrap/
    ...

    Just because you don't see it doesn't mean there is no use case. ;)
     
  21. Offline

    Wolvereness Bukkit Team Member

    What part of that is not covered by the API? Let's fix it!
    Really, the UUID should change if an entity completely changes scope.
     
  22. Every part is covered. That wasn't my point, my point was without copying all that manually.
    But it doesn't completely change scope, it just changes the world. Again: If it wouldn't loose the UUID plugins which want to track the entity could do it, without a hack for the UUID change.
     
  23. Offline

    Lolmewn

    Phew, that's quite a list you got there!
     
    -_Husky_- likes this.
  24. Offline

    EdGruberman

    Because these things are rarely, if ever, "simple". It is why Leaky exists. It is an official tool to manage such requests. Why should a developer be given special treatment to circumvent the process? Frustration with lack of success to date does not dictate that the Bukkit Team should react differently.

    @Wolverness - Your flow chart is useful, if still a bit scattered. However, I'm a bit confused by the "Am I doing something I'm not supposed to do?" assessment phase. It seems subjective. Do you have any objective/technical criteria that clarifies how the Bukkit Team makes such a decision?
     
  25. Offline

    Wolvereness Bukkit Team Member

    /e-hug
    Here are a few direct "these are never endorsed" items:
    1. Using reflection
    2. Compiling against the CraftBukkit jar
    3. Shading in a library without refactoring it to your own namespace
    4. Compiling against a library not located in Bukkit and without shading it (see #3) (exception: a plugin you depend on shades said library)
    5. Not using depend / softdepend to interact with another plugin's namespace (this has to do with the class searching methodology) (also, circular dependencies are bad...)
    6. Initializing variables outside of onLoad / onEnable (this means, calling getConfig() or similar as one of your variables in JavaPlugin, and a few other situations)
    7. Directly accessing System.in or System.out or anything similar (this includes using a logger not defined by JavaPlugin.getLogger())
    8. /humor: Misspelling the name of the person you are attempting to tag
     
  26. Offline

    bergerkiller

    (so this is still going on?)
    It is true that BKCommonLib starts to look like the older BukkitContrib and even Spout Plugin (successor) and many other libraries that exist these days. I also know that hacking into CraftBukkit can cause bugs. Which is why:
    • I have try-catches wherever possible something sensitive could go wrong
    • I have proper messages everywhere if something goes wrong
    • I put all sensitive stuff into BKCommonLib to prevent loose plugins being incompatible
    • I try to provide alternative ways (that are a lot slower) to reflection-based systems
    Now to give you a few examples for why I have to work in the net.minecraft.server namespace on a per-plugin level:

    TrainCarts
    Since the Bukkit Minecart entity does not have an onTick (obviously) I have to use EntityMinecart in the nms namespace. I have to use reflection to copy all fields from a minecart to my extended version. (ClassTemplate) I had to rewrite all of the minecart physics logic to work using train physics, to ignore certain collisions (the one in Bukkit fails), to have a proper staged onTick and more. I replace the EntityTrackerEntry with my own extended version to synchronize the entire train in one stage, in order to prevent bumping. This requires both a classtemplate transfer and a field to swap the entity tracker entry.

    NoLagg
    Extreme case. It uses a completely new list for the chunks component to handle chunk sending. (chunkSendQueue is replaced using reflection). It cancels all explosions and uses a new explosion model in the TNT comp. It uses a WorldListener to listen for entity adding and removing in the spawn limiter and stack former, as a tick task freezes the server with too many entities. It uses a lot of reflection to replace tasks with timed versions to examine. Lighting operates on full nms Chunk objects. Saving calls multiple nms methods to work.

    SignLink
    Replaces TileEntitySign with an extended version to prevent the text packet from being sent, so text can be made virtually instead. This required a lot of handling to properly replace tiles.

    MyWorlds
    Have to use reflection to properly close file streams when worlds unload. Had to replace the PlayerData instance in the ServerConfigurationManager thing to properly handle multi-world inventories. This involved multiple nms logic routines, like loading/saving player inventories.

    I guess the only exceptions to the rule are Stream Remover and Redstone Mania. They use a lot of Bukkit methods, because luckily I was able to. For the above four, however, net.minecraft.server is required because it has to either:
    • Replace entities or tile entities
    • Optimally listen for entity changes on the server (there is no entity add/remove event)
    • Handle things like packets and synchronization differently
    • Fix internal net.minecraft.server bugs caused by rapid injecting of CraftBukkit code before (world unloading should never be possible, so it has a lot of bugs as a result)
    All I can say in conclusion: I am proud that I managed to do things outside Bukkit's scope. I am glad that I can use CraftBukkit for more than chat plugins, block protection, entity/sound/particle spawning and /give plugins. It is only a shame that a lot of the things I use net.minecraft.server for could easily be added in the API. Copying data from one entity to the other, for example. Teleporting cross-world IS needed, think of portals. Listening for add/remove entity, to handle spawn limits and check if a certain type of entity is on the server so you can manipulate it in a tick task.

    EDIT

    I might actually be willing to make a PR replacing the many spawn/despawn events with two add and remove events. With backwards compatibility if you like. However, I do want to know the thoughts about this...
     
  27. Offline

    TnT

    Once again, please submit PR's for those items that, as you state "could easily be added in the API". We are all busy. The best way to get additions into the code that you want to see is to provide a PR, a leaky ticket, and a well thought out use case for those items. If those are truly easy additions, it should be easy for you to submit a PR for them.
     
  28. Offline

    bergerkiller

    TnT
    Ok, I will do that at some point then. I already messed around and turned the 'WorldListener' into a separate Entity Add and Entity remove events. All of my plugins now use it, and so far so good. Adding it into CraftBukkit is amazingly easy, so when I have some spare time, I will submit a PR to do it.

    (adding the event is no more but adding the event in the 'notifyEntityAdd/Remove' function in nms.World)
     
Thread Status:
Not open for further replies.

Share This Page