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

Merged

Conversation

danorris709
Copy link
Contributor

@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
Contributor 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
Contributor 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
Contributor 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

@danorris709 danorris709 force-pushed the tick-event-remove-phase-variable branch from e43cf89 to 5a9a327 Compare May 31, 2024 11:05
@LexManos LexManos merged commit 8f17370 into MinecraftForge:1.20.x Jun 13, 2024
2 checks passed
PaintNinja added a commit to PaintNinja/MinecraftForge that referenced this pull request Sep 3, 2024
…e#9890 to 1.20.4)

Co-Authored-By: Daniel Norris <33832062+danorris709@users.noreply.github.com>
PaintNinja added a commit that referenced this pull request Sep 12, 2024
….4) (#10090)

Co-authored-by: Daniel Norris <33832062+danorris709@users.noreply.github.com>
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.

3 participants