Server icon indicating copy to clipboard operation
Server copied to clipboard

[RFC] porting to sol2

Open mackal opened this issue 2 years ago • 9 comments

This replaces luabind with sol2.

There was a single breaking change, this can be fixed by running find . -name "*.lua" -exec sed -i '/.*\.entries/s/for \(.*\) in \(.*\) do/for _, \1 in pairs\(\2\) do/' "{}" \;

The pattern like

  for mob in mob_list.entries do
    if (mob.valid) then
      print(mob:GetName())
    end
  end

had to be replaced with

    for _, mob in pairs(mob_list.entries) do
        if (mob.valid) then
            print(mob:GetName())
        end
    end

mackal avatar Sep 18 '22 00:09 mackal

Found another breaking change, sol2 binds the usertype's constructors to a new function, I think PEQ had a single use of this, which is easy to fix.

mackal avatar Sep 20 '22 19:09 mackal

Some specific things I wanted comments on. In order to avoid potential memory consumption issues from templates I split up the implementation of the Lua_foo wrappers and the register functions. I initially ran with naming them like lua_client_impl.cpp since they're implementation files! But that's not really standard, lua_client.h is implemented in lua_client.cpp, generally. But I ran with it since I already started. Personally I think putting the register function in say lua_client_register.cpp would make more sense, but I wanted to bring it up first.

Another thing I think we should do eventually is add a perl and lua subfolder to zone and move the quest specific implementations into them. But that should probably happen in a different PR.

mackal avatar Sep 23 '22 18:09 mackal

I'll take a look at this guy in the next day or so

Akkadius avatar Sep 26 '22 04:09 Akkadius

What was done to convert the API? Did you go through every single one manually? Did you leverage any re-usable code generation?

What kind of testing have you done? I see there are a lot of different API's eg tables/states - have we checked for any inconsistencies in behaviors?

Generally excited to make the move, I need to get sleep its way too late. I'll look more tomorrow

Akkadius avatar Sep 28 '22 09:09 Akkadius

Did you leverage any re-usable code generation?

no

What kind of testing have you done?

I ran around did some quests, tested some encounters, spammed #reload quest in various zones. More quests need to be tested, but it isn't the easiest thing to test. I turned quest logging to gmsay on by default to make sure I wouldn't miss errors.

I see there are a lot of different API's eg tables

LuaBind has tables too, just they were passed around in generic objects, there isn't a difference.

mackal avatar Sep 28 '22 14:09 mackal

Plan to sit down again soon. After we get PEQ stable with current master we should then do testing with this branch

Akkadius avatar Oct 04 '22 13:10 Akkadius

Easiest way to test would probably be enable Quests logging to file and just grep for errors (probably GM say logging too if a GM is logged in) tell some PEQ players to do some quests and raids

mackal avatar Oct 04 '22 15:10 mackal

Or Discord Connector for all quest errors, that what I did and it shined a light on so many quest errors that I didn't know about.

fryguy503 avatar Oct 04 '22 15:10 fryguy503

Or Discord Connector for all quest errors, that what I did and it shined a light on so many quest errors that I didn't know about.

we should probably have that for normal PEQ anyways, and if we do, I must have the channel muted, there are too many!

mackal avatar Oct 04 '22 15:10 mackal

Coming back to this

Master Stability

Master had a lot of issues at the time preventing from being able to safely baseline and test something large like this.

PEQ has proven master has been largely stable and ready to test a branch like this

I noticed you closed this out of likely lack of desire to keep it up to date with master changes.

Sol2 Use

First of all, any change like this is great - but execution of a flawless migration for everyone is just as important. I've heard nothing but great thing about sol2 and we could benefit to modernize our lua binding library if executed flawlessly and it would be great to have that done since we've been talking about it for years.

Migration Concerns

Server operators shouldn't be expected to go in and make manual fixes to their scripts, it should all just work and would expect you to own any and all issues that arise from a result of said work. That is the main detractor to this sort of work getting to the finish line. There is a sed command but that is not cross-platform.

My expectation would be to not expect operators to solve the issues that come up from the massive surface area that this touches because they simply are not going to be equipped to solve them, will become frustrated and trust is lost

I liken this to when headings were changed in the source and then largely broken in many places for years to follow. Combat code froze updates froze operators from making code updates for several years and other people had to make up the difference to come up with solutions to move forward. We can't have that here and it needs to be a smooth migration.

  • The script conversion(s) needs to be cross platform
  • We need a way to catch issues automatically to build confidence in the migration before merging
  • Issues need to be continually owned and triaged beyond merge to master to ensure an experience for all operators until stable
  • Spire Quest API documentation will need to be updated to conform to the new source syntax (I'm fine with doing this or @Kinglykrab can, either way)

I'm happy to help you with facilitating testing and push this forward, but you need to own all the issues that come along with it and ensure stability at the end of it all

Otherwise - given everything was done flawlessly - this would be a big W

Akkadius avatar Nov 16 '22 14:11 Akkadius