Add swap function to util.maplist.EntityList

[bergerkiller]

Is your feature request related to a problem? Please describe. In BKCommonLib, I have to swap entities when implementing controllers / custom entities. There is one extra place where I need to swap, which is in the EntityList stored in the NMS Chunk (entities). Unfortunately, it does not implement a List interface so it's not a simple matter of setting at an index. Instead I have to remove, and re-add, the entity.

Describe the solution you'd like public boolean swap(Entity oldEntity, Entity newEntity)

Describe alternatives you've considered Right now I'm just calling remove(oldEntity) and then add(newEntity). This is less efficient and may break in the future if you decide to add events/other stuff here.

Additional context This is not critical, I'll be adding support for both old and new methods if a new one is ever to exist. Note, both entities will have the same entity id.

https://github.com/bergerhealer/BKCommonLib/blob/master/src/main/java/com/bergerkiller/bukkit/common/internal/logic/EntityAddRemoveHandler_1_14.java#L86

[aikar]

Please don't do this, as it will corrupt state is going to cause issues. the only correct way to do it is to remove the old entity from the world (not just the chunk!) and register the new one.

We set various state on objects we expect to be there (and Vanilla), and when you do what you are doing, any expectations will now be blown, and you're likely going to trigger illegal state warnings in logs.

Please just world.unregisterEntity(old) then world.registerEntity(new) to get 100% accurate behavior (same goes for Spigot)

[bergerkiller]

the only correct way to do it is to remove the old entity from the world (not just the chunk!) and register the new one.

I would do this, if it wouldn't cause events to fire all over the place, with packets being sent to destroy and recreate the entity. Losing linkage with the original Bukkit entity will break compatibility with a lot of other plugins. Swapping an entity has never been officially supported, but that's fine, I've supported this on my end for nearly a decade.

We set various state on objects we expect to be there

And I have utility functions to detect all such places in a serverwide sweep, so none will be forgotten as you make updates to the server.

The entity is in fact replaced everywhere, not just the chunk like you assumed. The replacement has 100% the same base type and the same field values. It's only purpose is to override methods like onTick that need to be hooked into.

[aikar]

are you using reflection to ensure every field is copied? if so i guess that would do it... but still iffy. But you could just set the original bukkit entities reference too.

But ultimately, this isn't something for paper to support as we dont want to encourage this behavior.

Also your current solution is perfectly fine, as the class notes, removes are O(1) (the reason for this change), and stuff going in and out of that EntityList object would not be a source for events (that would be the chunk methods itself)

[bergerkiller]

are you using reflection to ensure every field is copied?

Yup. Thats baked into Mountiplex's ClassInterceptor create function. This involves a lot of at-runtime code generation too. So public fields are simply assigned instead of using reflection.

But you could just set the original bukkit entities reference too.

That's done also yeah.

But ultimately, this isn't something for paper to support as we dont want to encourage this behavior.

Understandable. The thing is, this has been this way well before any sort of fork, even Spigot, existed. With just CraftBukkit around this is quite maintainable, but I'm starting to run into issues with all these new forks as of late. (Tuinity, Purpur. Theres forge too now-)

So yeah, I realize I'm on the edge of what is supported. I'm in luck that so far, in all places where an Entity is stored, the access doesn't require synchronization because it's all done on the main thread.

Ideally I would want the server itself to offer support for controllers so I don't have to hook anything. Just something so I can catch NMS Entity onDamage, onTick, etc. But hey, I've been waiting 7 years for that to happen, not going to wait on it ;)

But fair enough, no need to support what I'm doing as it's an incredibly niche application. If there ever ends up being an official API to instrument an Entity I'll be sure to use it.

[aikar]

Well with the direction mojang is going for Behaviors on Villagers, I can see demand for a Behaviors API come as im sure mojangs going to move all entities over to that, but that might be a few more years who knows.

[bergerkiller]

I look towards Sponge's mixins as a future I'd like to see. BKCommonLib's entity controllers are loosely based on the controller system from way-back Sponge that I was also a contributor for. But mixins are a lot more than that, and would allow multiple plugins to override behaviors without interfering.

Mojang isn't focused on a modding API anymore unfortunately. Forge exists, so they don't consider this something worth working on. That some parts of NMS happen to be quite moddable is more of a coincidence I feel.

And since Spigot is unwilling to make radical changes to the server because it would hurt their time-to-release, I feel CraftBukkit is kind of stuck in the past right now. I mean, I can support all Minecraft versions 1.8 - today in a single jar file, that should tell something...

[Spottedleaf]

The last thing we need is an easy way for plugins to insert code into methods that we don't expect as the "recommended" way of making changes in NMS.

[bergerkiller]

Meh, you'll see instantly it's modified code because the plugin's custom classes show up in the stack traces. In my case, it's the first thing I needed, because there was no other way back then to completely disable vanilla minecart physics and substitute it with my own.

I was messing with a tick task trying to alter velocity, position and angles to control it. But I couldn't override and disable entity collisions, causing two minecarts in a train to collide and stop the train. And the only solution was to rip out all the vanilla code.

This is still the case to this day. I have a completely rewritten move() function that has collision handling, because CraftBukkit in all these years never added this! The vehicle block/entity collision events are a joke, because cancelling it does nothing.

There's still plugins using vanilla entities to create particles, all wasting time doing block and entity collisions. Daily I have people asking me about performance, and I see armorstands and other entities wasting away valuable server tick time.

[bergerkiller]

Anyhow, this got very off-topic /rant

Sorry for that.