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

Core: Workflow to generate RPC headers #6179

Closed
wants to merge 1 commit into from
Closed

Conversation

zach2good
Copy link
Contributor

@zach2good zach2good commented Aug 27, 2024

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Very quickly threw this together in my lunch break.

  • Adds build dependency on Python, and enforces v3
  • Adds tools/generate_rpc_stubs.py and makes CMake watch for changes to it - so it can regenerate headers
  • Runs tools/generate_rpc_stubs.py during configuration and outputs to the cmake build folder
  • Adds that generation folder to global includes

Still deciding on how the RPC itself will work

Build instructions already specify you need python 3 installed, so we're covered there. This acts as a hard check to make sure you have it now too

@HealsCodes
Copy link

Should a "Very quickly threw this together in my lunch break. [...]
Still deciding on how the RPC itself will work" really be a PR at all??

At this stage it feels like you want to try something new and fancy but are lacking a fundamental plan or direction how to get there - this should live on a private branch until it's thought through and provides a benefit for the project.

@zach2good
Copy link
Contributor Author

zach2good commented Aug 29, 2024

Hey @HealsCodes!

This pull request is still a work in progress
Draft pull requests cannot be merged.

This PR

In terms of my thought process for this PR, and future PRs, it's a very early implementation step for this issue #6165 (which it just slipped my mind to link when I opened the PR).

The main goal of this PR as stated in the title and those bullet points, is not to replace the RPC yet, but to introduce the Python dependency to the main build and to put some plumbing in place for script running (and therefore header generation) at configure time. This is absolutely something that I can throw together on my lunch break. Currently, the resultant output headers aren't very useful, but they go straight into the build output folder and aren't used.

This also opens the door for automatic db maintenance on build, so people won't trip themselves up with "oh no, I forgot to run dbtool after updating and all my stuff is broken".

Lacking direction

If you look at my PR history, you'll see that I open a lot of PRs in draft very early in their lifecycle, and iterate on them in public so that people can weigh in on my thought process. So, thanks for being one of the few who's actually taken the time to weigh in, I do appreciate it!

I take a strong stance against people hoarding code that they claim to want to release to the public "at some point", and this workflow is my way of demonstrating that I do all of my development painfully in public. If I were to get hit by a bus tomorrow, I'd like my thought process and progress on all of this stuff to be clearly recorded and available for anyone who might want it. Sure, I could do that as part of the commits and commit messages (and arguably that's better), but this is a more user friendly way of presenting that information.

Lack of thought/care

With other huge architectural changes (sol refactor, instance system refactor, CI updates, navmesh speedups, dynamic entities, custom menus, modules & module build system, new db system), we don't yeet them into the codebase without sufficient iteration and testing. We're more than aware that this live service software that people take updates from daily, and that if we push out unreasonable bugs during development it'll immediately come back to bite us and we'll have to scramble to get fixes out to restore user workflows.

Future of RPC in this repo

I won't explain to you all the issues we have in the repo about performance and how difficult and fiddly our RPC/message passing is, I'm sure you're already very aware. The long term vision is to make it not horrendous. Currently, after years of talking about it and thinking how we might pull it off, it's looking like we'll finally bite the bullet and split zone and entity ticks into awaitable tasks to be run on an executor using coroutines. Interleaving blocking ZMQ/SQL/Navmesh will be a huge boon to the server process, boosting the amount of zones and players we can host on a single process.

This does mean though that the plumbing will be quite hard for non-C++ users to reason about; so a code generation step at configure time is meant to alleviate that and allow more Lua-focused users to participate.

@zach2good
Copy link
Contributor Author

I'm supplanting this PR with this one: #6359

Where I'm going to rewrite the current manual message serialization between all processes with an auto-generated one, and then I'll build RPC on top of it.

@zach2good zach2good closed this Oct 17, 2024
@zach2good zach2good deleted the zmq_rpc_generation branch October 17, 2024 10:55
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