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

Refactor Event Queues #5671

Merged
merged 6 commits into from Jan 7, 2024
Merged

Conversation

Web-eWorks
Copy link
Member

@Web-eWorks Web-eWorks commented Nov 25, 2023

I had a long description typed out, then Github decided to eat it. As a result you get the short description instead.

This PR adds C++17-based template parameter pack overloads to push multiple values to Lua, refactors our Event module, and adds a separate UI event queue which is updated at all times - even in the menu and while the game is paused.

Hooking into the new queue is easy:

ui.Events.Register("onGamePaused", function()
    print("I've been paused!")
end)

You can create your own event queues, too!

pi_lua_import(l, "Event");
LuaRef eq = LuaTable(l, -1).Call<LuaRef>("New");

// ... make it available to Lua somehow

// Queue events using the familiar LuaEvent API
LuaEvent::Queue(eq, "myShinyEvent", ...);

// Make sure to call _Emit regularly or events won't be processed
ScopedTable(eq).Call("_Emit");

You can now call require 'Event':_Dump() in the console to print a list of events and the modules which have registered handlers for them. This should aid in debugging and refactoring.

image

This PR also adds support for hot-reloaded modules overwriting their old event handler definitions too. As long as the new version of the module registers the same set of event listeners, the old ones will be silently swapped out during the call to Event.Register. This gets us one step closer to a much richer hot-reload experience working on all areas of the game code.

CC @JonBooth78 / @fluffyfreak - I see we have a manual fallback path for MSVC with the pi_lua_multiple_push functions in LuaPushPull.h. Can that fallback path be removed and migrated to C++17 fold expressions safely without breaking MSVC builds?

@Max5377 this should unblock #5644 (at long last, sorry for the delay). PiGui was the best spot I found to hang a general-purpose "user interface" queue off of, and while I have mixed feelings about the thought of tying the music player to the UI code I think this is probably the best way to handle it unless you have a better idea for an API point to make this queue available at.

- Add LuaTable::PushBack / PushMultiple method for appending to the end of an array table
- Add LuaPushMultiple to push a parameter pack onto the stack
- package.modulename(stacklevel) requests the module id for the function N levels above it in the stack
- Can be used to cache the module ID for a function or callback, for example
- Support arbitrary event parameter counts via template parameter packs
- Queue new events at the end of the array to match Event.lua
- Support pushing events to multiple event queues
- Perf: don't call pi_lua_import every time we queue an event to Lua

- Ship.cpp no longer depends on a transitive include for Pi.h
- PiGui event queue for all user-interface relevant events coming from C++ or Lua
- Updated every frame during the menu and game regardless of paused state
@Max5377
Copy link
Contributor

Max5377 commented Nov 25, 2023

Just gonna say, thank you for doing this PR. I don't really have much to say about music player being part of the UI and
I think nothing terrible will happen if it stays like this for now.

- Variadic push/pull is implemented with c++17 fold expressions and initializer lists for defined execution order
@Web-eWorks
Copy link
Member Author

I've pushed an experimental commit that replaces the chained function calls and fallback paths for old MSVC versions with a template parameter-pack based solution. From what I could see, evaluation order in a braced-init-list constructor (for std::tuple) is well-defined and does what we want; obviously, the comma operator in a fold expression also has a defined execution order.

I'd appreciate some feedback from those on the Windows side of things regarding how well MSVC handles this change.

@JonBooth78
Copy link
Contributor

Hey @Web-eWorks , sorry life got in the way for a while there, I should have some time on Sunday to test this out - I'm happy to build and run it; do you have any test areas/things I should test on MSVC?

Oh, and this looks good :-)

@Web-eWorks
Copy link
Member Author

@JonBooth78 testing focus should be on pushing multiple-parameter events through the LuaEvent system - I'd like to make sure that the fold-expressions are correctly pushing arguments in order and that overloads/template parameters are being resolved correctly.

My testing so far has been to fix the cases where it's obviously wrong and type errors have been thrown and then to fly around for 10 minutes or so. If you have a save from a few commits ago it'd be worth testing save/load and general post-game-start behaviors.

@JonBooth78
Copy link
Contributor

OK, sorry @Web-eWorks for the delay :-(

So I think I should look at events that already push multiple arguments, such as the ship ones; onShipDestroyed, onShipCollided, onShipScoopedFuel etc.

I might also drop in a couple of new events and push them, with multiple different arguments of various types and check that they get received by Lua in the correct order.

@JonBooth78
Copy link
Contributor

OK, I've hacked in some test code here: JonBooth78@8d6dff3 which is very ugly but I think tests what needs to be tested and is working as expected.

@Web-eWorks , does that look like a valid test of the what you were after, anything else I could add?

An older save file appears to be working. I'll have a crack at creating a new game in a moment, but so far, so good.

@Web-eWorks
Copy link
Member Author

Absolutely no worries about the delay. I've been busy enough between finally writing v2 of the equipment system and personal life that I completely forgot this PR was still open until yesterday... so perfect timing!

That test harness looks good - I primarily needed confirmation that my understanding of someone else's understanding of the C++ standard did in fact mean that the major compilers we build Pioneer with have a well defined order-of-operations with regards to pushing arguments to Lua.

If no further issues arise from your testing, I'll go ahead and merge this sometime tomorrow (in 12-16 hours).

@Web-eWorks
Copy link
Member Author

Narrator: it was not in fact tomorrow.

Thanks for the testing, I'm generally satisfied that if issues arise from this they would have failed the smoke testing both you and I performed.

@Web-eWorks Web-eWorks merged commit 6821097 into pioneerspacesim:master Jan 7, 2024
4 of 5 checks passed
@Web-eWorks Web-eWorks deleted the game-event-queue branch January 7, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants