Skip to content

Conversation

Phill310
Copy link
Contributor

@Phill310 Phill310 commented Aug 29, 2024

Description

Add worldborder syntax to Skript. Continuation of #5229
Player's worldborder is always virtual

Conditions:

  • updated IsWithin condition

Effects:

  • expand/shrink worldborder (maybe add grow/contract aliases)
    I don't like the way I am registering the syntax right now with the of and 's but I can't think of a different way

Events:

  • Worldborder bounds change
  • Worldborder bounds finish change
  • Worldborder center change

Expressions:

  • worldborder
  • worldborder center
  • worldborder damage amount
  • worldborder damage buffer
  • worldborder size
  • worldborder warning distance
  • worldborder warning time

Sections:

  • Create worldborder

Target Minecraft Versions: any
Requirements: Paper for worldborder events
Related Issues: #5178, #3848

Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

TEST!

@Fusezion
Copy link
Contributor

A lot of chez's stuff might of been repeated he beat me ;(

@sovdeeth sovdeeth added the feature Pull request adding a new feature. label Aug 30, 2024
@Phill310 Phill310 marked this pull request as ready for review September 9, 2024 01:55
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

just needs some formatting updates, mainly around annotations!

import org.jetbrains.annotations.Nullable;

@Name("Warning Time of World Border")
@Description("The warning time of a world border. If the border is shrinking, the player's screen will be tinted red once the border will catch the player within this time period.")
Copy link
Member

Choose a reason for hiding this comment

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

so if the player is outside the border it will be tinted red after one second? im not sure what this expression does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its specifically when the worldborder is shrinking. If the player is outside the final worldborder (after the shrink), their screen will turn red x seconds before it reaches their current location

Copy link
Contributor

@Absolutionism Absolutionism left a comment

Choose a reason for hiding this comment

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

Just a few things

Location location = mode == ChangeMode.SET ? (Location) delta[0] : null;
for (WorldBorder worldBorder : getExpr().getArray(event)) {
switch (mode) {
case SET:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but maybe an enhanced switch?

Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Leaving a proper review to other people, but uhh thoughts on WorldBorder x AnySizedThing? size of worldborder, set size of worldborder to 10

Copy link
Contributor

@Absolutionism Absolutionism left a comment

Choose a reason for hiding this comment

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

Looking good

@@ -53,6 +53,7 @@
import org.bukkit.event.player.PlayerQuitEvent.QuitReason;
import org.bukkit.event.player.PlayerResourcePackStatusEvent.Status;
import org.bukkit.event.player.PlayerTeleportEvent.TeleportCause;
import org.bukkit.event.player.PlayerExpCooldownChangeEvent.ChangeReason;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you've had some trouble merging/updating your branch, so let's hope nothing got removed in the making.

Comment on lines 54 to 61
if (((long) worldBorder.getWarningDistance() + input) > Integer.MAX_VALUE) {
worldBorder.setWarningDistance(Integer.MAX_VALUE);
} else if (((long) worldBorder.getWarningDistance() + input) < Integer.MIN_VALUE) {
worldBorder.setWarningDistance(Integer.MIN_VALUE);
} else {
worldBorder.setWarningDistance(Math.max(worldBorder.getWarningDistance() + input, 0));
}
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (((long) worldBorder.getWarningDistance() + input) > Integer.MAX_VALUE) {
worldBorder.setWarningDistance(Integer.MAX_VALUE);
} else if (((long) worldBorder.getWarningDistance() + input) < Integer.MIN_VALUE) {
worldBorder.setWarningDistance(Integer.MIN_VALUE);
} else {
worldBorder.setWarningDistance(Math.max(worldBorder.getWarningDistance() + input, 0));
}
break;
int current = worldBorder.getWarningDistance();
int value = Math2.fit(0, current + input, Integer.MAX_VALUE);
worldBorder.setWarningDistance(value)

Shouldnt be using MIN_VALUE because that's a negative, I dont think a world border should have a negative distance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MIN_VALUE part is true but I did the rest of it to catch integer overflows during the addition / subtraction. It seems like the Math2.fit wouldn't help that unless I am misunderstanding the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Well one thing you should do, is in #acceptChange instead of returning Number.class you should do Integer.class in my opinion. As that in itself would exclude any nan and infinite values. As well as capping the provided number to Integer.MAX_VALUE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont think switching it to integer class fixed the integer overflow issues so I think it might leave it as is with the MIN_VALUE thing fixed

@Phill310
Copy link
Contributor Author

Phill310 commented Mar 8, 2025

Leaving a proper review to other people, but uhh thoughts on WorldBorder x AnySizedThing? size of worldborder, set size of worldborder to 10

What do you mean? Is this some newer feature that I haven't seen yet / forgot about?

Copy link
Contributor

@Absolutionism Absolutionism left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

looks good to me
i appreciate the very thorough tests

@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Mar 15, 2025
@Efnilite Efnilite requested a review from a team as a code owner March 20, 2025 13:25
@Efnilite Efnilite requested review from cheeezburga and erenkarakal and removed request for Fusezion March 20, 2025 13:25
@Efnilite Efnilite merged commit 1e43779 into SkriptLang:dev/feature Mar 20, 2025
5 checks passed
@Phill310 Phill310 deleted the feature/worldborder branch March 26, 2025 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants