Add elytra boost event, firework type and firework syntaxes #5956

TheLimeGlass · 2023-08-31T03:06:55Z

Description

Adds Paper's elytra boost event.
Makes Firework a ClassInfo.
Adds Firework syntaxes.

Target Minecraft Versions: 1.18 for some 1.19 for some
Requirements: Paper for some syntaxes
Related Issues: #5826

Fusezion

Only gone through this class too distracted rn to do more

src/main/java/org/skriptlang/skript/elements/fireworks/ExprFireworkFlownTime.java

src/main/java/ch/njol/skript/Skript.java

Fusezion

Some other changes found, and concerns.

src/main/java/org/skriptlang/skript/elements/fireworks/ExprFireworkFlownTime.java

src/main/java/org/skriptlang/skript/elements/fireworks/CondElytraBoostConsume.java

src/main/java/org/skriptlang/skript/elements/fireworks/ExprAttachedTo.java

Fusezion

Couldn't find anything else to criticize so looks good to me.

src/main/java/ch/njol/skript/Skript.java

src/main/java/org/skriptlang/skript/elements/fireworks/CondElytraBoostConsume.java

src/main/java/org/skriptlang/skript/elements/fireworks/EffElytraBoostConsume.java

src/main/java/org/skriptlang/skript/elements/fireworks/ExprAttachedTo.java

src/main/java/org/skriptlang/skript/elements/fireworks/ExprFireworkFlownTime.java

AyhamAl-Ali

Meant to RC but commented instead

…achedTo.java Co-authored-by: Ayham Al Ali <[email protected]>

sovdeeth · 2024-03-21T12:05:48Z

src/main/java/org/skriptlang/skript/elements/fireworks/CondElytraBoostConsume.java

+@Examples({
+	"on player elytra boost:",
+		"\twill consume firework",
+		"\tset to consume the firework"


invalid example

sovdeeth · 2024-03-21T12:06:50Z

src/main/java/org/skriptlang/skript/elements/fireworks/CondElytraBoostConsume.java

+
+	static {
+		if (Skript.classExists("com.destroystokyo.paper.event.player.PlayerElytraBoostEvent"))
+			Skript.registerCondition(CondElytraBoostConsume.class, "[event] (will [:not]|not:won't) consume [the] firework");


I think it should be forced to include what is consuming the firework:

"[the] (event|player) (will [:not]|not:won't) consume [the] firework"

sovdeeth · 2024-03-21T12:08:03Z

src/main/java/org/skriptlang/skript/elements/fireworks/CondElytraBoostConsume.java

+		if (!(event instanceof PlayerElytraBoostEvent))
+			return false;
+		return isNegated() && !((PlayerElytraBoostEvent) event).shouldConsume();


Suggested change

if (!(event instanceof PlayerElytraBoostEvent))

return false;

return isNegated() && !((PlayerElytraBoostEvent) event).shouldConsume();

if (event instanceof PlayerElytraBoostEvent)

return ((PlayerElytraBoostEvent) event).shouldConsume() ^ isNegated();

return false;

sovdeeth · 2024-03-21T12:09:58Z

src/main/java/org/skriptlang/skript/elements/fireworks/EffElytraBoostConsume.java

+@Examples({
+	"on player elytra boost:",
+		"\twill consume firework",
+		"\tset to consume the firework"


invalid example

sovdeeth · 2024-03-21T12:10:27Z

src/main/java/org/skriptlang/skript/elements/fireworks/EffElytraBoostConsume.java

+
+	static {
+		if (Skript.classExists("com.destroystokyo.paper.event.player.PlayerElytraBoostEvent"))
+			Skript.registerEffect(EffElytraBoostConsume.class, "[not:do not] consume firework", "set consum(e|ption of) firework to %boolean%");


set consume of firework shouldn't be allowed.
also, what do you think about [un]cancel consumption of [the] firework or something similar?

sovdeeth · 2024-03-21T12:17:34Z

src/main/java/org/skriptlang/skript/elements/fireworks/ExprFireworkFlownTime.java

+	"How much time that the fireworks have been boosting the player in an <a href='events.html#elytra_boost'>elytra boost event</a>.",
+	"Maximum is the same as time until detonation. Paper names them differently than Spigot."


Suggested change

"How much time that the fireworks have been boosting the player in an <a href='events.html#elytra_boost'>elytra boost event</a>.",

"Maximum is the same as time until detonation. Paper names them differently than Spigot."

"How long a fireworks has been boosting the player in an <a href='events.html#elytra_boost'>elytra boost event</a>.",

"Maximum is the same as time until detonation."

Also, having maximum life time be the same as time until detonation is a poor naming scheme we shouldn't pass on. The maximum life time should be from the launch until detonation, and the time until detonation should be from the current moment until detonation.

sovdeeth · 2024-03-21T12:18:35Z

src/main/java/org/skriptlang/skript/elements/fireworks/ExprFireworkFlownTime.java

+			return Timespan.fromTicks_i(detonate ? firework.getTicksToDetonate() : firework.getTicksFlown());
+		return Timespan.fromTicks_i(detonate ? firework.getMaxLife() : firework.getLife());


Suggested change

return Timespan.fromTicks_i(detonate ? firework.getTicksToDetonate() : firework.getTicksFlown());

return Timespan.fromTicks_i(detonate ? firework.getMaxLife() : firework.getLife());

return Timespan.fromTicks(detonate ? firework.getTicksToDetonate() : firework.getTicksFlown());

return Timespan.fromTicks(detonate ? firework.getMaxLife() : firework.getLife());

sovdeeth · 2024-03-21T12:21:22Z

src/main/java/org/skriptlang/skript/elements/fireworks/ExprFireworkFlownTime.java

+
+	@Override
+	public void change(Event event, @Nullable Object[] delta, ChangeMode mode) {
+		long ticks = (mode == ChangeMode.DELETE || mode == ChangeMode.RESET) ? 0 : ((Timespan) delta[0]).getTicks_i();


Suggested change

long ticks = (mode == ChangeMode.DELETE || mode == ChangeMode.RESET) ? 0 : ((Timespan) delta[0]).getTicks_i();

long ticks = (mode == ChangeMode.DELETE || mode == ChangeMode.RESET) ? 0 : ((Timespan) delta[0]).getTicks();

sovdeeth · 2024-03-21T12:21:35Z

src/main/java/org/skriptlang/skript/elements/fireworks/ExprFireworkFlownTime.java

+				ticks = -ticks;
+			case ADD:
+				for (Firework firework : getExpr().getArray(event)) {
+					long existing = convert(firework).getTicks_i();


Suggested change

long existing = convert(firework).getTicks_i();

long existing = convert(firework).getTicks();

sovdeeth · 2024-03-21T12:26:26Z

src/main/java/org/skriptlang/skript/elements/fireworks/ExprFireworkFlownTime.java

+			case DELETE:
+			case RESET:
+				for (Firework firework : getExpr().getArray(event)) {
+					if (detonate) {
+						if (PAPER) {
+							firework.setTicksToDetonate((int) 20 * firework.getFireworkMeta().getPower());
+						} else {
+							firework.setMaxLife((int) 20 * firework.getFireworkMeta().getPower());
+						}
+					} else {
+						if (PAPER) {
+							firework.setTicksFlown((int) 20 * firework.getFireworkMeta().getPower());
+						} else {
+							firework.setLife((int) 20 * firework.getFireworkMeta().getPower());
+						}
+					}
+				}
+				break;


I think this is unintuitive.
Delete should not be a valid changer, in my opinion, or if it is, it should set the value to 0.
Reset can't reliably figure out what the lifespan should be, since it doesn't know how long it's flown already.
For that reason, Reset probably also shouldn't be valid, or if it is, it should set remaining time to the original amount and the ticks flown to 0.

sovdeeth · 2024-09-15T20:18:05Z

closing due to inactivity

TheLimeGlass added 2 commits August 30, 2023 00:31

Start

143486a

Add elytra boost event and firework type

8ede96f

TheLimeGlass added enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature Pull request adding a new feature. labels Aug 31, 2023

License

fee096b

Fusezion suggested changes Aug 31, 2023

View reviewed changes

Fusezion reviewed Aug 31, 2023

View reviewed changes

src/main/java/ch/njol/skript/Skript.java Show resolved Hide resolved

Apply changes

e524be6

TheLimeGlass requested a review from Fusezion August 31, 2023 03:19

Fusezion suggested changes Aug 31, 2023

View reviewed changes

License

c340ba5

TheLimeGlass requested a review from Fusezion August 31, 2023 03:47

TheLimeGlass and others added 3 commits August 30, 2023 21:51

Apply changes

24f275c

Apply changes

1b43a87

Merge branch 'master' into feature/elyta-boost-event

d16f423

Fusezion approved these changes Aug 31, 2023

View reviewed changes

AyhamAl-Ali mentioned this pull request Aug 31, 2023

Add WILL to property condition #5955

Merged

AyhamAl-Ali reviewed Aug 31, 2023

View reviewed changes

AyhamAl-Ali requested changes Aug 31, 2023

View reviewed changes

Update src/main/java/org/skriptlang/skript/elements/fireworks/ExprAtt…

1987c0f

…achedTo.java Co-authored-by: Ayham Al Ali <[email protected]>

TheLimeGlass requested a review from AyhamAl-Ali August 31, 2023 16:01

Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59

TheLimeGlass changed the base branch from master to dev/feature October 7, 2023 05:15

TheLimeGlass requested review from sovdeeth, APickledWalrus and UnderscoreTud March 14, 2024 17:12

TheLimeGlass added 2 commits March 14, 2024 11:25

Update to dev/feature

d156a9e

Sorted imports

34a8971

sovdeeth requested changes Mar 21, 2024

View reviewed changes

sovdeeth closed this Sep 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add elytra boost event, firework type and firework syntaxes #5956

Add elytra boost event, firework type and firework syntaxes #5956

TheLimeGlass commented Aug 31, 2023

Fusezion left a comment

Fusezion left a comment

Fusezion left a comment

AyhamAl-Ali left a comment

sovdeeth Mar 21, 2024

sovdeeth Mar 21, 2024

sovdeeth Mar 21, 2024

sovdeeth Mar 21, 2024

sovdeeth Mar 21, 2024

sovdeeth Mar 21, 2024

sovdeeth Mar 21, 2024

sovdeeth Mar 21, 2024

sovdeeth Mar 21, 2024

sovdeeth Mar 21, 2024

sovdeeth commented Sep 15, 2024

		"How much time that the fireworks have been boosting the player in an <a href='events.html#elytra_boost'>elytra boost event</a>.",
		"Maximum is the same as time until detonation. Paper names them differently than Spigot."

		return Timespan.fromTicks_i(detonate ? firework.getTicksToDetonate() : firework.getTicksFlown());
		return Timespan.fromTicks_i(detonate ? firework.getMaxLife() : firework.getLife());

	long ticks = (mode == ChangeMode.DELETE \|\| mode == ChangeMode.RESET) ? 0 : ((Timespan) delta[0]).getTicks_i();
	long ticks = (mode == ChangeMode.DELETE \|\| mode == ChangeMode.RESET) ? 0 : ((Timespan) delta[0]).getTicks();

	long existing = convert(firework).getTicks_i();
	long existing = convert(firework).getTicks();

Add elytra boost event, firework type and firework syntaxes #5956

Add elytra boost event, firework type and firework syntaxes #5956

Conversation

TheLimeGlass commented Aug 31, 2023

Description

Fusezion left a comment

Choose a reason for hiding this comment

Fusezion left a comment

Choose a reason for hiding this comment

Fusezion left a comment

Choose a reason for hiding this comment

AyhamAl-Ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sovdeeth commented Sep 15, 2024