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

 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

+ 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

 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.

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

 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

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

+ 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

+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

+ 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.

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

+ * 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


- 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

Nuutraiand 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 ce85a90April 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

Uh oh!

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