Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Add Configuration for vertical Despawn Ranges

Open notTamion opened this issue 10 months ago • 8 comments

resolves #10177 config naming/structure might need some changes, i didn't wanna break existing configs so i just did it like this.

currently doesn't work for Raiders as those have extra checking logic in the removeWhenFarAway method, so i am not quite sure what to do about those

EDIT: also wanted to add that i am not sure if the normal despawn range should still impact the y axis. since your x axis is going to be higher then your y axis 99 % of the time anyways i will just have it keep affecting that aswell

notTamion avatar Apr 20 '24 15:04 notTamion

I'm not sold on the format for the config with this change. There are several options that I can think of that I'd prefer

despawn-ranges:
    monster:
        hard:
            xz: 30 # or "horizontal"
            y: 5 # or "vertical"
        soft: 20 # just putting a numerical value would mean it applies to both (as it does now)

or

despawn-ranges:
    monster:
        hard: 30
        soft: 20
        soft-y: 5 # basically the `-y` value wouldn't even be in the config if it wasn't being used

Machine-Maker avatar Apr 20 '24 16:04 Machine-Maker

is there an existing way i can add optional configurations like that?

notTamion avatar Apr 20 '24 17:04 notTamion

As you already said, this does not currently "work", as the distanceSqrt still takes into account the y diff. Given we already have changes to basically every line in this section, I think it would be fair to just completely rewrite this logic in a new paper block without consideration of keeping old code around/minimizing diff (given that this isn't minimal diff either, every line is touched beyond the two discard calls).

This would also heavily improve the variable names, making this whole mess a lot easier to read.

lynxplay avatar Apr 20 '24 18:04 lynxplay

alright i will probably be working on the custom serializer and the rewrite tomorrow

notTamion avatar Apr 20 '24 20:04 notTamion

Implemented the requested changes

notTamion avatar Apr 22 '24 13:04 notTamion

bump Is there a chance to merge before 1.21 arrives 1717882987914

CatTeaA avatar Jun 08 '24 21:06 CatTeaA

if someone could comment on whether or not the config layout is ok that would be great, then i can quickly writeup a docs pr so that's also ready.

new config layout (as proposed by machine):

despawn-ranges:
    monster:
        hard:
            horizontal: 30
            vertical: 5
        soft: 20 # just putting a numerical value would mean it applies to both (as it does now)

notTamion avatar Jun 09 '24 08:06 notTamion

if someone could comment on whether or not the config layout is ok that would be great, then i can quickly writeup a docs pr so that's also ready.

new config layout (as proposed by machine):

despawn-ranges:
    monster:
        hard:
            horizontal: 30
            vertical: 5
        soft: 20 # just putting a numerical value would mean it applies to both (as it does now)

I think this config format layout is great Clear and straightforward

This PR can perfectly solve the problem I'm currently encountering. https://github.com/PaperMC/Paper/discussions/10520

CatTeaA avatar Jun 09 '24 13:06 CatTeaA

Sorry, another bump, the stable version 1.21 has been released for some time now, can this feature be merged now? I really need it.

CatTeaA avatar Aug 12 '24 08:08 CatTeaA

Brought this up internally, but I am less and less convinced that this "dynamic" syntax is that nice for us. Configs would now potentially change outside of a version migration because someone defined

hard:
  xy: 10
  y: 10

Would obviously be interested in other peoples input here, but a single migration and simply always using the more verbose layout would imo work just as fine.

lynxplay avatar Aug 14 '24 15:08 lynxplay

Would configs changing node structure have any kind of impact though? configurate still only sees that Map<MobType, DespawnRange> and all (de)serialization regarding the underlying nodes is done by the serializer. Do you have any example of how this might have an bad impact?

notTamion avatar Aug 14 '24 16:08 notTamion

I mean, mostly that the file changes without a migration notice in the _version field.

Now that I look at it, we'd also instead want to make it use default instead of the current versions magic values. And current layout brings a couple of annoying side effects. When you only define hard: default, the config makes sense. All ranges for hard despawning are defaulted. If you insert

hard:
  horizontal: 10

then vertical is implicitly set to default, even without it being present in the config. So now we are introducing a) paper changing the config file without the need to do so, potentially breaking/messing with server state managers (at least mine lol) b) it computes an implicit value.

Beyond the fact that we need to change this to using "default" values, need to drop the ConfigSerializable annotation and also need to register the deserializer in the first place, I think it may be worth to (at least) look into preserving the config structure if people defined the same value for horizontal/vertical.

lynxplay avatar Aug 14 '24 16:08 lynxplay

I can definitely change it so it doesn't doesn't create a node with a default value if it hasn't been defined in the config and change it so it uses default instead of the normal numbers. _version is there to make sure configs aren't loaded on incompatible versions (which i just realized i also have to up considering old servers won't play nice with this) not to track config history. I will just wait on this pr for some time and allow people to give their thoughts on it.

notTamion avatar Aug 14 '24 17:08 notTamion

Yea probably best, I'll through it at my list to discuss with machine whenever we next sit down. But yea, if you could register the serialiszer and yank the config seria annotation, that would be great. Just so that no one forgets that xD

lynxplay avatar Aug 14 '24 17:08 lynxplay

Already did that yesterday, see the latest commit. (not even sure whether i forgot to re add that on the 1.21 or 1.21.1 update lol)

notTamion avatar Aug 14 '24 18:08 notTamion

Ah sweet, I must have pulled an out of date version then when I was looking over it today, sorry!

lynxplay avatar Aug 14 '24 18:08 lynxplay

Going further this PR should

a) emit xz, y fields if specified in config, even if their values match. For this DespawnRange needs to track if the val was defined in long or short syntax. (Preferably new type that is Range(xz, y, longSyntax)

b) require defining vertical if horizonal was defined. No "partial" long syntax.

lynxplay avatar Aug 16 '24 19:08 lynxplay