Allow a pending entity ID to be applied at configuration end (internal)

[AhmadNasser04]

Allow an external service to own the players entity id by exposing an api to set the players entity id during configuration.

[electronicboy]

entity ids are now per world, I don't exactly understand the usecase for this outside of stuff we generally don't want to support, but as-is this is fairly dangerous

[mbax]

If messing with entity IDs yourself, wouldn't you want to be just bumping the next ID the same way the server does, instead of this? Wouldn't this also be an issue for non-player entities?

[Owen1212055]

Is this for seemless transitions?

[AhmadNasser04]

Is this for seemless transitions?

Correct it is for seemless transitions, I want to make my velocity proxy own the entity id as im not sending the join game packet to the client anymore

[lynxplay]

Is this something we should plug into UnsafeValues if we decided to pull something like this.
E.g. via a UnsafeValues#reserveEntityId(PlayerConfigurationConnection, long)?
I do not care to expose entity ids into the public API and this API can literally break everything with a wrong call.

[DebitCardz]

Is this something we should plug into UnsafeValues if we decided to pull something like this. E.g. via a UnsafeValues#reserveEntityId(PlayerConfigurationConnection, long)? I do not care to expose entity ids into the public API and this API can literally break everything with a wrong call.

I think putting this API in UnsafeValues would be a good call, this API does seem good for people with specialized use-cases but I agree making this a public api someone can accidentally mess with seems like a bad call.

[AhmadNasser04]

Is this something we should plug into UnsafeValues if we decided to pull something like this. E.g. via a UnsafeValues#reserveEntityId(PlayerConfigurationConnection, long)? I do not care to expose entity ids into the public API and this API can literally break everything with a wrong call.

I've went ahead and moved this to UnsafeValues#reserveEntityId(PlayerConfigurationConnection, int), I do agree that its a better spot to have something like this

[Malfrador]

IMO this should not be part of the API, even in UnsafeValues. Way too easy to break the server in fun ways. And UnsafeValues in general is a bit of a questionable concept to have in general, we really should not add even more to that mess.

I am fine with just the NMS parts of it though. That would put it in a similar position as the ChannelInitializeListener "API", which is also only intended for packet-modifying stuff.

[AhmadNasser04]

IMO this should not be part of the API, even in UnsafeValues. Way too easy to break the server in fun ways. And UnsafeValues in general is a bit of a questionable concept to have in general, we really should not add even more to that mess.

I am fine with just the NMS parts of it though. That would put it in a similar position as the ChannelInitializeListener "API", which is also only intended for packet-modifying stuff.

I mean the alternative to allowing this is listening on the LoginEvent and using NMS/paperweight to modify the entity id of the player yourself then.

But the LoginEvent is deprecated & causes the player to be created prematurely, this simply allows you to do the same thing in a safe manner where the player is not created prematurely and the flow is generally the same.

Like it's "easy to break the server" but I just don't know how much I love that line of thinking as you quickly end up restricting developer freedom from doing some cool stuff, I mean if someone breaks the server by using a colliding entity id that's their fault and it's clearly documented

I also really don't want to run a paper fork just for this small change lmfao

[ProdPreva1l]

lgtm

[lynxplay]

Like it's "easy to break the server" but I just don't know how much I love that line of thinking as you quickly end up restricting developer freedom from doing some cool stuff, I mean if someone breaks the server by using a colliding entity id that's their fault and it's clearly documented

What malfrador suggested here is to maintain this as internal "API". E.g. your plugin would require paperweight userdev to effectively do what the PR rn does in UnsafeValues. It is effectively another "beware here be dragons" guard for the method. We'd keep the additional field and methods on the configuration connection impl and have it be applied but it just isn't exposed at all to the normal API but instead requires you to opt into internals via paperweight.

I can kinda see malfs point too, UnsafeValues, while doing unsafe things, generally still is "safe" but possibly breaking and might return unexpected things. I don't have strong feelings either way, but if usage of paperweight userdev is fine for you that would probably make the most people in here happy xD Otherwise we'll grab some other team members for review, I think unsafe values can work fine as well.

You'd effectively be listening to whatever event you would with this PR but just cast the connection to its impl yourself to call the setPendingEntityId method.

[kennytv]

How do you plan on managing entity ids on the proxy and avoiding conflict on the backend, both for existing and then-added entities? Knowing more details of the actual use makes designing this easier in a way that isn't either too convoluted or too dangerous to expose publicly

[AhmadNasser04]

Like it's "easy to break the server" but I just don't know how much I love that line of thinking as you quickly end up restricting developer freedom from doing some cool stuff, I mean if someone breaks the server by using a colliding entity id that's their fault and it's clearly documented

What malfrador suggested here is to maintain this as internal "API". E.g. your plugin would require paperweight userdev to effectively do what the PR rn does in UnsafeValues. It is effectively another "beware here be dragons" guard for the method. We'd keep the additional field and methods on the configuration connection impl and have it be applied but it just isn't exposed at all to the normal API but instead requires you to opt into internals via paperweight.

I can kinda see malfs point too, UnsafeValues, while doing unsafe things, generally still is "safe" but possibly breaking and might return unexpected things. I don't have strong feelings either way, but if usage of paperweight userdev is fine for you that would probably make the most people in here happy xD Otherwise we'll grab some other team members for review, I think unsafe values can work fine as well.

You'd effectively be listening to whatever event you would with this PR but just cast the connection to its impl yourself to call the setPendingEntityId method.

Yeah I'm fine with user paperweight for this, as long as the issue of needing to listen on the LoginEvent is addressed & not creating the player prematurely.

Considering that, I've went ahead and made this internal only so there is no longer a public api for it, and you need paperweight userdev to modify it. As long as there is a "proper" way to do this then I'm happy :)

Although if you believe this would get approved with UnsafeValues I'd be happy to put it back there as well

[AhmadNasser04]

How do you plan on managing entity ids on the proxy and avoiding conflict on the backend, both for existing and then-added entities? Knowing more details of the actual use makes designing this easier in a way that isn't either too convoluted or too dangerous to expose publicly

I increment starting from 1.6 billion instead of 1, so realistically should never collide as the paper backend would have to create 1.6 billion entities before the ranges could collide. Is there a safer way to do this? Probably, but for my use case it should be good enough

[ProdPreva1l]

i have a suggestion to
make this safer, also expose entity id creation for normal entities to userdev so it can be overridden and handled the same way (using proxy as SSOT)

[AhmadNasser04]

@lynxplay don't mean to be a bother but i closed this PR by mistake and re-opened it, but the project automation bot kept its status in closed in the paper pr queue 😛