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

SkriptTimings - massive overhaul #7420

Draft
wants to merge 13 commits into
base: dev/feature
Choose a base branch
from

Conversation

ShaneBeee
Copy link
Contributor

@ShaneBeee ShaneBeee commented Jan 11, 2025

Description

This PR aims to do a massive overhaul of Skript's timing system.

Warning

This is a very VERY rough draft of what can be done

As many know, Paper has disabled their built in timings and moved over to use Spark timings.
Skript's timings relied solely on Paper's timings and have been rendered useless.

I have spent much time finicking around with this just to see what could be done.
A lot of the code here is just an example of what COULD be done, but may be a bad way of doing it.

CHANGES:

  • Created an abstract Timings class which can be implemented by another class
    • This could be overridden by an add-on
    • Not that this will ever happen, but let's pretend Spark added custom timing support, Skript could create a SparkTimings class.
  • SkriptTimings is no longer a static class. It now extends Timings and is setup in Skript's main class.
  • Created an interface Timeable (sp?) which is implemented in a few classes to be able to retrieve Nodes.
    • I couldn't think of another way to force nodes into these objects so my implementation is probably not good
    • Nodes are needed for getting lines.
  • Timings are done per script and per line (reloading a script will reset the timings for said script)
  • Command/TabCompletion for the timings are done within the Timings class.
    • This allows the timing implementation to handle the command how they wish.

COMMAND:

I made a command (with a print function in the SkriptTimings class) just to test how the timings were working.
The print function is god awful but it was all I could think of for now.
Each line is given a timing.

  • Black lines are commented out lines, ex: # blah blah
  • Dark grey lines are lines that haven't been timed (whether they don't have a timing or just never got executed)
  • Light grey lines are timed
    • The lines are appended with the following (average ms per execution [timing count])
    • Colors:
      • Green = Timing is good, under 20ms
      • Yellow = Timing is ok, 20-35ms
      • Orange = Timing isn't so good, 35-50ms
      • Light Red = Timing is going to lag, 50-100ms
      • Dark Red = EEEEK, over 100ms
      • Blue = Timing is async and shouldn't affect performance.

The gif just shows the command being run after different resets and timings:
ezgif-2-c058518072

I am open to feedback and seeing where this could be taken.

Reminder: VERY VERY VERY ROUGH DRAFT!


Target Minecraft Versions: any
Requirements: none
Related Issues: none

@Moderocky
Copy link
Member

Looks like a really good start, this is something I've wanted to see for a while.

- fixes a bug where stopping a loop (in a script) would not stop the timing
@ShaneBeee
Copy link
Contributor Author

Looks like a really good start, this is something I've wanted to see for a while.

Thank you.
I really think something like this would be so handy for Skripters. I also think it's really needed.
I have seen many many many people over the years struggle with finding where performance issues are coming from.

That said, there are a few things here I am not happy with (ex: getting the nodes).

I would love to see this added one day (or something similar) so im definitely open to feedback in making this PR better.

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

Successfully merging this pull request may close these issues.

2 participants