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

Configurable Per-World Void Damage Height Offset & Amount #11436

Merged
merged 6 commits into from
Sep 29, 2024

Conversation

Axionize
Copy link
Contributor

No description provided.

@Axionize Axionize requested a review from a team as a code owner September 25, 2024 08:59
CONTRIBUTING.md Outdated Show resolved Hide resolved
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.

Welcome to paper 🎉
Thank you for your first PR.

Left a couple of comments that need addressing before something like this can be merged.

@Axionize Axionize force-pushed the configurable-void-height-settings branch 5 times, most recently from dc5875b to 6e85ecf Compare September 26, 2024 23:19
@Axionize
Copy link
Contributor Author

I've fixed all the issues you've shown me and changed the PR to be an offset from minBuildHeight() as you said you wanted.

@Axionize Axionize changed the title Configurable Per-World Void Damage Threshold Height & Amount Configurable Per-World Void Damage Height Offset & Amount Sep 27, 2024
@lynxplay
Copy link
Contributor

Do you have a specific use-case for adding API getters/setters?
We'd rather hold off on adding more methods until we have a strong API specific usecase, given this seems rather admin specific with the config values.

The API should already be able to deal void damage via the damage cause API if needed and these methods add to the general clutter that is the World interface already.

@lynxplay
Copy link
Contributor

Beyond that, we can combine enabled and damage into a FloatOr.Disabled (needs to be created)

@Axionize
Copy link
Contributor Author

Axionize commented Sep 28, 2024

Do you have a specific use-case for adding API getters/setters?
We'd rather hold off on adding more methods until we have a strong API specific use-case, given this seems rather admin specific with the config values.

Yes, dynamically loading/building worlds with WorldCreator from region files on disk, mainly for minigames.

Currently you can generate a custom world at runtime with a custom generator, biomeProvider, etc... but obviously void kill height can not be set when creating the world, much less when the world is loaded.

An alternative may be to modify WorldCreator.java to support passing in a custom world.yaml config settings (and potentially reloading those settings at runtime in World.java), but this is obviously much simpler and I would very much enjoy not having to take on such a task.

The API should already be able to deal void damage via the damage cause API if needed and these methods add to the general clutter that is the World interface already.

Yes but as far as I'm aware there is no way to change the actual damage delt by being in the void for an entity vs a plugin applying damage and setting the DamageCause as Void, for example by some RPG plugins.

You would have to check if damage cause is void, check if the player's y level is below the y-level of whatever the void damage height is, and if the damage was within a certain epsilon of 4.0F and then change the amount of damage applied to the player, to achieve the same result reliably.

@lynxplay
Copy link
Contributor

Eh sounds fair, I guess.
I'll chat it over with the rest of the team.

You can deal void damage perfectly fine via https://jd.papermc.io/paper/1.21.1/org/bukkit/entity/Damageable.html#damage(double,org.bukkit.damage.DamageSource) and https://jd.papermc.io/paper/1.21.1/org/bukkit/damage/DamageType.html#OUT_OF_WORLD respectively tho.

Maybe it is worth consolidating the void damage configuration into a single interface type, e.g.

World#getVoidDamageConfiguration()
World#setVoidDamageConfiguration(WorldVoidDamageConfiguration)

interface WorldVoidDamageConfiguration {

boolean isVoidDamageEnabled();
double getVoidDamagePerTick();
float getMinWorldHeightOffset();

static WorldVoidDamageConfiguration vanilla() { }
static WorldVoidDamageConfiguration enabled(double damage, float minWorldHeightOffset) { }
static WorldVoidDamageConfiguration disabled() { }
}

but I'll get back to concrete next steps once we have more team input on this.
For now, could you please address machines comment regarding the "threshold".
It cannot be a constant y-value, it should be expressed as an offset from the min build height, as the default world configuration otherwise breaks custom height worlds.

@Axionize Axionize force-pushed the configurable-void-height-settings branch from cb2dd00 to 38d328c Compare September 28, 2024 21:01
@Axionize
Copy link
Contributor Author

I've made it an offset from the build height as requested, defaulting to -64 blocks from the minBuildHeight.

@Machine-Maker Machine-Maker force-pushed the configurable-void-height-settings branch from 38d328c to dfed6f8 Compare September 29, 2024 21:21
@lynxplay lynxplay force-pushed the configurable-void-height-settings branch from 43d872c to 4644865 Compare September 29, 2024 21:58
@lynxplay lynxplay merged commit b410fe8 into PaperMC:master Sep 29, 2024
3 checks passed
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.

4 participants