Events rehaul by Nuutrai · Pull Request #16 · LuaRocket/rocket

Nuutrai · 2025-03-10T06:41:47Z

This is a big event rehaul focused on allowing for dynamic events

I'm writing this in a sort of a rush as I need to go to sleep, however if you have any questions, feel free to ask as many as you need, and I'll get back to you as soon as I can.

Thank you!

Nuutrai · 2025-03-10T06:42:21Z

The class loading also needs to disable loading for abstract classes, but that can wait

zNotChill · 2025-03-10T17:19:44Z

from and to are part of the PlayerMoveEvent event.

Co-authored-by: zNotChill <znotchill@znci.dev> Co-authored-by: mibers <midaswoah2@gmail.com>

# Conflicts: # src/main/kotlin/dev/znci/rocket/scripting/events/EventListener.kt

When first experimenting with this type of event handling, I found that there was an issue where every event was called regardless of whether it appeared in any scripts. This fixes this. Note that currently, disabling does not yet remove events from the handler, so that will be next

Nuutrai · 2025-03-14T18:11:12Z

Note that currently, disabling does not yet remove events from the handler, so that will be one of the next parts

…unctions (future)

Someone should work on this in the future, I'll try to remember to make an issue for it

Signed-off-by: Lexî Nuutraî <info@nuutrai.com>

zNotChill

Pretty much looks good to me, however unnecessary prints should be removed.

Nuutrai · 2025-03-18T20:21:01Z

Whoops, should I just revert the debug commit altogether?

This reverts commit e681fd0.

zNotChill · 2025-03-18T20:22:52Z

If you want to, as long as it doesn't break anything. You could just commit again with the debugs removed. Either way would work as long as nothing breaks.

Nuutrai · 2025-03-18T20:25:24Z

Let me know if that's better

Signed-off-by: Lexî Nuutraî <info@nuutrai.com>

gibbiemonster · 2025-04-08T12:18:22Z

Merging is blocked pending #32.
There are also new merge conflicts with the New API. Please resolve these how you see fit!

# Conflicts: # build.gradle.kts # src/main/kotlin/dev/znci/rocket/Rocket.kt # src/main/kotlin/dev/znci/rocket/scripting/ScriptManager.kt

Nuutrai · 2025-04-08T20:39:25Z

I'm not entirely sure what other things need to be changed regarding the API since most of it is mainly file stuff

gibbiemonster · 2025-04-10T14:27:52Z

@Nuutrai is this ready for review?

zNotChill · 2025-04-10T15:35:18Z

build.gradle.kts

    implementation("net.luckperms:api:5.4")
    implementation("org.jetbrains.kotlin:kotlin-reflect")
+    // TODO check if needed
+    implementation("com.google.guava:guava:32.0.1-jre")


@blockarchitech said not to use guava, he should also comment here

zNotChill · 2025-04-10T15:43:07Z

src/main/kotlin/dev/znci/rocket/scripting/globals/tables/Events.kt

+        var previousChar: Char? = null
+        var record = false
+        for (char in function.toString()) {
+            if ((previousChar ?: ' ') == ':' && char == ':') if (record == false)  record = true else break


Solve the Qodana issue

zNotChill

Looks mostly good to me, obviously needs to be tested, however there is some issues that should be addressed first

gibbiemonster

More info in the comments. I would generally ignore the Guava comments, on the condition we're planning to use Guava in the future.

Otherwise, do also solve the Qodana issues. There aren't a whole lot, though 🙂

Nice work!

gibbiemonster · 2025-04-10T15:05:23Z

build.gradle.kts

    implementation("net.luckperms:api:5.4")
    implementation("org.jetbrains.kotlin:kotlin-reflect")
+    // TODO check if needed
+    implementation("com.google.guava:guava:32.0.1-jre")


I think guava is a welcome addition but I see not usages of it here.

src/main/kotlin/dev/znci/rocket/Rocket.kt

gibbiemonster · 2025-04-10T15:06:30Z

src/main/kotlin/dev/znci/rocket/commands/RocketCommand.kt

        val action = args[0].lowercase()
-        val scriptName = if (!args[1].endsWith(".lua")) "${args[1]}.lua" else args[1]
+        val rawScriptName = args[1]
+        val scriptName = if (!rawScriptName.endsWith(".lua")) "${rawScriptName}.lua" else rawScriptName


We may want to sanitize this to prevent directory traversal.

Could you explain that further?

Could you explain that further?

Directory traversal can mostly be prevented by removing any /es from the start, and ../ from the entire path. ./ is fine because it doesn't move to any directory

src/main/kotlin/dev/znci/rocket/commands/RocketCommand.kt

gibbiemonster · 2025-04-10T15:08:08Z

src/main/kotlin/dev/znci/rocket/commands/RocketCommand.kt

+                    val results = ScriptManager.loadAll()
+                    if (results.isNotEmpty()) {
+                        results.forEach { error ->
+                            sender.sendMessage(LocaleManager.getMessageAsComponent("generic_error", error ?: "Unknown error"))


@zNotChill - we need better error handling 🙂

Want to open a ticket for that?

@zNotChill - we need better error handling 🙂

Want to open a ticket for that?

On my new branch (not yet published), I have already mostly done this. Anywhere you use a RocketError will now work, whereas before it would not, and it would just produce a error calling _: nil error. Now it actually shows you what you did wrong.

gibbiemonster · 2025-04-10T15:14:31Z

src/main/kotlin/dev/znci/rocket/scripting/events/EventListener.kt

+        eventCallbacks.forEach { callback ->
+            val luaTable = convertEventToLua(event)
+            callback.call(luaTable)
+            println(callback)


Avoid println.

This will be removed before merging as it was mainly for debugging

gibbiemonster · 2025-04-10T15:15:19Z

src/main/kotlin/dev/znci/rocket/scripting/events/EventListener.kt

+    private fun getSupportedEvents(): HashSet<Class<out Event>> {
+        val set = hashSetOf<Class<out Event>>()
+        for (eventClass in getBukkitEventClasses()) {
+            // TODO Add a debug setting to show these kinds of things


Leads to some of the QoL things I bug @zNotChill about 😁

There is a way to check if a debugger is attached, and thus, add some trace logs, but I haven't looked too much into implementation.

src/main/kotlin/dev/znci/rocket/scripting/events/EventListener.kt

gibbiemonster · 2025-04-10T15:18:09Z

src/main/kotlin/dev/znci/rocket/scripting/events/EventListener.kt

+     *          Maybe for a different PR, it depends on what znci is feeling
+     */
+
+    private fun convertEventToLua(event: Event): RocketTable {


I do not like this. Can we convert to using a class so it's more portable? I defer to the greater team however this looks like a testing nightmare and I do not like how much this still uses the old API. I also am not a fan of the giant switch statement.

As in a class rather than an event? I'm still looking into the new API, so I'll try to get those changed soon. What would you prefer instead of the switch statement?

I agree, I don't really see what could be changed about the switch statement..

I think we should rethink how this entire class works. It should be rewritten with the New API's capabilities in mind. I will elaborate in #dev...

gibbiemonster · 2025-04-10T15:18:37Z

src/main/kotlin/dev/znci/rocket/scripting/events/EventListener.kt


-        return luaTable
+        })
+        table.setmetatable(meta)


I recall a while ago we decided not to use the metatable for some reason?

Without the meta table, users can't have dynamic event values, or at least stringified event values. We'll hopefully add support for conversions though (just a thought)

I meant directly. The new API fills out a lot of this stuff for you.

# Conflicts: # build.gradle.kts # src/main/kotlin/dev/znci/rocket/scripting/ScriptManager.kt # src/main/kotlin/dev/znci/rocket/scripting/events/EventListener.kt

Nuutrai added 2 commits March 10, 2025 01:17

I think it works

80a4731

Failed attempts at debugging (will keep trying)

b67f31c

Nuutrai and others added 5 commits March 10, 2025 22:39

Made it more dynamic with the previous way of retrieving values

969ad03

Co-authored-by: zNotChill <znotchill@znci.dev> Co-authored-by: mibers <midaswoah2@gmail.com>

Merge remote-tracking branch 'upstream/main' into events-rehaul

2408029

# Conflicts: # src/main/kotlin/dev/znci/rocket/scripting/events/EventListener.kt

A couple of small fixes

3524629

No idea if this works, mainly just putting this onto GitHub

fceb00b

Nuutrai added 8 commits March 16, 2025 07:21

Move to a new system to *finally* give tracking to events and other f…

9271572

…unctions (future)

Adjust LuaEvents to use new system

4894dde

Adjust EventListener to use new system

2c98a1c

Make the disable subcommand work

ddc17e6

Fix config reloading (maybe)

2448a5e

Very rudimentary debug system (see details)

e681fd0

Someone should work on this in the future, I'll try to remember to make an issue for it

Remove todo because it's added (yay!)

5880019

Adjust TODO message

405274e

Nuutrai marked this pull request as ready for review March 16, 2025 15:07

Nuutrai requested a review from zNotChill as a code owner March 16, 2025 15:07

Nuutrai added 3 commits March 16, 2025 16:10

Merge branch 'main' into events-rehaul

c9041f2

Signed-off-by: Lexî Nuutraî <info@nuutrai.com>

Not sure what happened here

f055863

Merge branch 'main' into events-rehaul

7962964

zNotChill requested changes Mar 18, 2025

View reviewed changes

Revert "Very rudimentary debug system (see details)"

2ae3316

This reverts commit e681fd0.

Remove unnecessary debugs

bbce335

Merge remote-tracking branch 'origin/events-rehaul' into events-rehaul

55a4537

Nuutrai requested a review from zNotChill March 18, 2025 20:49

Nuutrai added 2 commits March 18, 2025 23:54

Merge branch 'main' into events-rehaul

8d3a8d4

Signed-off-by: Lexî Nuutraî <info@nuutrai.com>

Use the spread operator ya dingus

dc9e582

zNotChill previously approved these changes Mar 21, 2025

View reviewed changes

Nuutrai added 2 commits March 23, 2025 01:21

Merge branch 'main' into events-rehaul

e3ab81a

Merge branch 'main' into events-rehaul

3ede5a0

gibbiemonster mentioned this pull request Apr 8, 2025

Technical Debt: Migrate Events to the New API #32

Open

gibbiemonster marked this pull request as draft April 8, 2025 12:18

Nuutrai added 2 commits April 8, 2025 15:53

Merge branch 'main' into events-rehaul

eceafc1

# Conflicts: # build.gradle.kts # src/main/kotlin/dev/znci/rocket/Rocket.kt # src/main/kotlin/dev/znci/rocket/scripting/ScriptManager.kt

Finish fixing conflicts

ce85a90

Nuutrai dismissed zNotChill’s stale review via ce85a90 April 8, 2025 20:15

Nuutrai requested a review from zNotChill April 8, 2025 20:17

Minor fixes to use Rocket API

00525ce

Nuutrai marked this pull request as ready for review April 10, 2025 15:31

zNotChill reviewed Apr 10, 2025

View reviewed changes

zNotChill requested changes Apr 10, 2025

View reviewed changes

gibbiemonster requested changes Apr 10, 2025

View reviewed changes

Nuutrai added 9 commits April 10, 2025 11:48

Qodana warnings

3ef5d79

Merge branch 'main' into events-rehaul

9311f8c

Switch scriptName variables

fd51bb8

Remove unused INSTANCE variable

a5b8f83

Merge remote-tracking branch 'origin/events-rehaul' into events-rehaul

55f40b5

Merge remote-tracking branch 'upstream/main' into events-rehaul

1c6bbbd

# Conflicts: # build.gradle.kts # src/main/kotlin/dev/znci/rocket/scripting/ScriptManager.kt # src/main/kotlin/dev/znci/rocket/scripting/events/EventListener.kt

Twine

b7e18eb

Removed invalid imports

3774d6d

Begin switch to Twine

0b03213

Conversation

Nuutrai commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nuutrai commented Mar 10, 2025

Uh oh!

zNotChill commented Mar 10, 2025

Uh oh!

Nuutrai commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zNotChill left a comment

Choose a reason for hiding this comment

Uh oh!

Nuutrai commented Mar 18, 2025

Uh oh!

zNotChill commented Mar 18, 2025

Uh oh!

Nuutrai commented Mar 18, 2025

Uh oh!

gibbiemonster commented Apr 8, 2025

Uh oh!

Nuutrai commented Apr 8, 2025

Uh oh!

gibbiemonster commented Apr 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zNotChill left a comment

Choose a reason for hiding this comment

Uh oh!

gibbiemonster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Nuutrai commented Mar 10, 2025 •

edited

Loading

Nuutrai commented Mar 14, 2025 •

edited

Loading