Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Configuration for vertical Despawn Ranges #10440

Merged
merged 4 commits into from
Aug 16, 2024

Conversation

notTamion
Copy link
Contributor

@notTamion notTamion commented Apr 20, 2024

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

Docs-pr at: PaperMC/docs#465

@notTamion notTamion requested a review from a team as a code owner April 20, 2024 15:28
@notTamion notTamion marked this pull request as draft April 20, 2024 15:33
@notTamion notTamion marked this pull request as ready for review April 20, 2024 15:49
@Machine-Maker
Copy link
Member

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

@notTamion
Copy link
Contributor Author

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

@lynxplay
Copy link
Contributor

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.

@notTamion
Copy link
Contributor Author

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

@notTamion notTamion force-pushed the add-config-for-y-despawn-ranges branch 2 times, most recently from d6f0c44 to 386f521 Compare April 21, 2024 06:13
@notTamion
Copy link
Contributor Author

Implemented the requested changes

@notTamion notTamion force-pushed the add-config-for-y-despawn-ranges branch from 862c67c to 679ed07 Compare April 28, 2024 09:13
@notTamion notTamion changed the title Add Configuration for y Despawn Ranges Add Configuration for vertical Despawn Ranges Apr 28, 2024
@notTamion notTamion force-pushed the add-config-for-y-despawn-ranges branch from fba9cdc to 036ccbd Compare April 28, 2024 12:39
@notTamion notTamion force-pushed the add-config-for-y-despawn-ranges branch from 036ccbd to 272c867 Compare May 13, 2024 17:20
@CatBang

This comment was marked as off-topic.

@notTamion
Copy link
Contributor Author

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)

@CatBang
Copy link

CatBang commented Jun 9, 2024

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.
#10520

@notTamion notTamion force-pushed the add-config-for-y-despawn-ranges branch 2 times, most recently from 1becb29 to 9bcc6f4 Compare July 13, 2024 19:34
@CatBang
Copy link

CatBang commented Aug 12, 2024

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.

@notTamion notTamion force-pushed the add-config-for-y-despawn-ranges branch 3 times, most recently from 29bdddc to a17c184 Compare August 12, 2024 11:29
lynxplay
lynxplay previously approved these changes Aug 13, 2024
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, however we will need to come up with a docs PR that correctly represents this more dynamic syntax.

@lynxplay
Copy link
Contributor

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.

@notTamion
Copy link
Contributor Author

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?

@lynxplay
Copy link
Contributor

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 lynxplay dismissed their stale review August 14, 2024 16:35

Needs more eyes and thoughts.

@notTamion
Copy link
Contributor Author

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.

@lynxplay
Copy link
Contributor

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

@notTamion
Copy link
Contributor Author

notTamion commented Aug 14, 2024

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)

@lynxplay
Copy link
Contributor

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

@lynxplay
Copy link
Contributor

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.

@notTamion notTamion force-pushed the add-config-for-y-despawn-ranges branch from d2b1c7d to 9c23a25 Compare August 16, 2024 20:25
@lynxplay lynxplay merged commit 1b8ab11 into PaperMC:master Aug 16, 2024
3 checks passed
lynxplay added a commit to lynxplay/paper that referenced this pull request Aug 16, 2024
lynxplay added a commit that referenced this pull request Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

Add a separate Y-value check for "despawn-ranges"
5 participants