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

Add new event TickRateChangedEvent #11479

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Axionize
Copy link
Contributor

@Axionize Axionize commented Oct 8, 2024

This event fires when the tick rate is changed, either by a command or by a plugin.

Useful if you have code dependent on the tickRate (my use case is simulating and predicting possible player movements) and you don't want to have to poll the tickRate all the time.

@Axionize Axionize requested a review from a team as a code owner October 8, 2024 05:47
@dawon
Copy link
Contributor

dawon commented Oct 8, 2024

Isn't the api/event itself missing?

@Axionize
Copy link
Contributor Author

Axionize commented Oct 8, 2024

Thank you for noticing that. Not used to the paper build system. Let me fix that real quick.

EDIT: rebuildPatches ate my code I'm going to have to rewrite this...

@masmc05
Copy link
Contributor

masmc05 commented Oct 8, 2024

Also check https://github.com/PaperMC/Paper/blob/master/CONTRIBUTING.md#pr-policy, you're both missing comments on changed lines and you have imports instead of using fully qualified name

@masmc05
Copy link
Contributor

masmc05 commented Oct 8, 2024

Also, use https://jd.papermc.io/paper/1.21.1/org/bukkit/event/Event.html#callEvent(), not the plugin manager

@Axionize
Copy link
Contributor Author

Axionize commented Oct 8, 2024

Pushed the actual event code

@Axionize
Copy link
Contributor Author

Axionize commented Oct 9, 2024

@masmc05 I addressed all the changes you requested. Added comments starting where my patch modifies code, switched to using event.callEvent() and the patch now uses the FQDN instead of imports for changes in Paper-Server.

@Axionize
Copy link
Contributor Author

Axionize commented Oct 15, 2024

I've addressed all issues that have been brought up. Is there anything else I need to do for this to be merged?

@Lulu13022002
Copy link
Contributor

The event mix jetbrains and jspecify nullable annotations. And it's a bit iffy when a plugin change the tick rate cause the command executor doesn't receive the right tick rate in the feedback message (unlike the WorldGameRuleChangeEvent for example)

@Axionize
Copy link
Contributor Author

I'll fix the notations but I don't understand your second concern, what exactly is the problem? That the message returned to the player won't reflect the new tick rate?

new file mode 100644
index 0000000000000000000000000000000000000000..279e471f00495cc08ed2297efb21fb801cd43851
--- /dev/null
+++ b/src/main/java/org/bukkit/event/server/TickRateChangeEvent.java
Copy link
Member

Choose a reason for hiding this comment

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

Paper events go in the paper package

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 was thinking about upstreaming this into Spigot later

Copy link
Member

Choose a reason for hiding this comment

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

if that's the case, then this PR is inherently blocked as it makes little sense for us to finalise the review and such of this into a state we're happy to merge it if we're going to potentially have to deal with breaking ABI changes from upstream

Copy link
Contributor

Choose a reason for hiding this comment

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

Which will inherently be the case as spigot does not expose the CommandSourceStack this event now provides.
Would be great if you can come to a decision whether or not you want to have this event in spigot-api soonish then. As cat mentioned, if this is going upstream, there is 0 point in this PR receiving reviews on paper and we can close it instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any update on this? I'd really like to either merge or close this depending on your decision regarding moving this to upstream.

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've decided to leave it in paper. Are there any other changes you'd like me to make?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well as cat stated, the event should be in the paper event package

@electronicboy
Copy link
Member

That the message returned to the player won't reflect the new tick rate?

Yes, the server prints the output of the tick rate, and yet, if a plugin mutates it, this value will be wrong; if we want to support mutation, the logic should be adjusted to get the new rate so that the message is displayed properly, rather than having invisible side-effects to end users

@Axionize
Copy link
Contributor Author

Axionize commented Oct 20, 2024

I've restructured this PR so that mutations will be reflected in messages and return values.

@Axionize Axionize requested a review from masmc05 October 23, 2024 13:07
@Axionize
Copy link
Contributor Author

Hi, is there anything left for me to do before this can be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: PR Party candidate
Development

Successfully merging this pull request may close these issues.

7 participants