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

ref(event): change tick event to have pre/post classes #9890

Open
wants to merge 7 commits into
base: 1.20.x
Choose a base branch
from

Conversation

danorris709
Copy link

@danorris709 danorris709 commented Mar 20, 2024

Removes the phase variable in the TickEvent class and changes all the sub classes to have Pre/Post classes, as mentioned in here

Theoretically this would give a minor performance boost due to the singleton nature of the ClientTickEvent, however as that was not the primary goal of this I've not included any numbers and only mention this as an extra benefit

@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.20 labels Mar 20, 2024
@autoforge autoforge bot requested a review from a team March 20, 2024 14:09
@PaintNinja PaintNinja added the BreakingChange This request includes a change that would break compatibility with current versions of Forge. label Mar 20, 2024
Copy link
Contributor

@PaintNinja PaintNinja 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 - makes it harder for devs to accidentally have their things run twice per tick and improves consistency with other events.

Approving but leaving Lex to decide if he wants to merge now or next MC version.

@autoforge autoforge bot added Assigned This request has been assigned to a core developer. Will not go stale. and removed Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. labels Mar 20, 2024
@LexManos
Copy link
Member

LexManos commented Mar 22, 2024

Throw some bin compat bouncers with @Deprecated(forRemoval = true, since = "1.20.4") and pull.
Few other notes, May be worth reformatting the event class to braces on same line like i've been doing while I go through.
Also super() calls are not needed as the no args call is implicit.

@PaintNinja PaintNinja marked this pull request as draft March 28, 2024 15:16
@danorris709
Copy link
Author

Throw some bin compat bouncers with @Deprecated(forRemoval = true, since = "1.20.4") and pull. Few other notes, May be worth reformatting the event class to braces on same line like i've been doing while I go through. Also super() calls are not needed as the no args call is implicit.

For the deprecated tag do you mean that I should restore the variable and deprecate it?

@LexManos
Copy link
Member

Yes doing so would allow it to be pulled in 1.20.4, however with 1.20.5 coming here soon it doesn't matter.

@PaintNinja PaintNinja marked this pull request as ready for review April 30, 2024 19:23
@autoforge autoforge bot requested a review from a team April 30, 2024 19:23
@PaintNinja PaintNinja removed the request for review from a team April 30, 2024 19:24
@danorris709
Copy link
Author

Yes doing so would allow it to be pulled in 1.20.4, however with 1.20.5 coming here soon it doesn't matter.

Alright I've rebased it for latest versions & added back the phases with a deprecated annotation too

@@ -38,78 +38,147 @@ public static class ServerTickEvent extends TickEvent {
private final BooleanSupplier haveTime;
private final MinecraftServer server;

public ServerTickEvent(Phase phase, BooleanSupplier haveTime, MinecraftServer server)
{
protected ServerTickEvent(BooleanSupplier haveTime, MinecraftServer server, Phase phase) {
Copy link
Member

Choose a reason for hiding this comment

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

All of these public -> protected are binary breaks.
Its fine to change them to protected {and move the phase to the end} but the public ones have to stay as bouncers.
And also you should mark the proper time for removal
@Deprecated(forRemoval = true, since = "1.20.6")

Maintaining binary compatibility means that your PR can be backported without major modifications

Suggested change
protected ServerTickEvent(BooleanSupplier haveTime, MinecraftServer server, Phase phase) {
@Deprecated(forRemoval = true, since = "1.20.6")
public ServerTickEvent(Phase phase, BooleanSupplier haveTime, MinecraftServer server) {
this(haveTime, server, phase);
}
protected ServerTickEvent(BooleanSupplier haveTime, MinecraftServer server, Phase phase) {

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes I completely missed that, thank you! I think I managed to get all of them now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Assigned This request has been assigned to a core developer. Will not go stale. BreakingChange This request includes a change that would break compatibility with current versions of Forge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants