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

Plugins v3 #472

Merged
merged 38 commits into from
Nov 6, 2023
Merged

Plugins v3 #472

merged 38 commits into from
Nov 6, 2023

Conversation

catornot
Copy link
Member

@catornot catornot commented Jun 18, 2023

yes

what changed

  • nuked presence ( will be move to the discord rpc plugin )
  • more exposed sq functions
  • exposed dll addresses
  • g_pCVar is exposed
  • added "userdata" to plugin's async call
  • added runframe to plugins

 

Related

@F1F7Y F1F7Y added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Jun 28, 2023
@Jan200101
Copy link
Contributor

Small wish I have:
make plugin_abi.h a pure C header.

Right now, plugins need to maintain the same data structure in their own codebase, making it rather tedious to maintain.
Turning plugin_api.h into a pure C header would allow languages that can understand C headers (e.g. C, C++, Zig) to import it directly, with little to no modification needed.

@catornot
Copy link
Member Author

catornot commented Aug 2, 2023

plugins that can be used to test plugins v3 :)
r2rcon-rs: r2rcon_rs.zip
rrplug_test: rrplug_test.zip
^ no src code

they don't work after b292371 commit

F1F7Y
F1F7Y previously requested changes Aug 2, 2023
Copy link
Member

@F1F7Y F1F7Y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as some changes I like, my main problem with our current plugin system is that increasing the version breaks all existing plugins.

Ill probably draft up an issue of how i think we could do better as increasing version on every minor change doesnt sound feasible at all

NorthstarDLL/plugins/plugin_abi.h Show resolved Hide resolved
NorthstarDLL/plugins/pluginbackend.cpp Outdated Show resolved Hide resolved
PLUGINS.ipch Outdated Show resolved Hide resolved
@GeckoEidechse
Copy link
Member

As much as some changes I like, my main problem with our current plugin system is that increasing the version breaks all existing plugins.

In the case of v3 here, it's more than just a minor change, right? ^^

Copy link
Contributor

@Jan200101 Jan200101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried testing with the given plugin but it just panics and gets the game stuck after the company logos

[2023-10-21] [08:35:45] [PLUGINSYS] [info] Succesfully loaded R2Northstar\plugins\r2rcon_rs.dll
[2023-10-21] [08:35:45] [PLUGINSYS] [info] Loading plugin r2rcon-rs version 1.0
[2023-10-21] [08:35:45] [r2rcon-rs] [info] plugin logging initialized
[2023-10-21] [08:35:45] [r2rcon-rs] [info] plugin static initialized : true
[2023-10-21] [08:35:45] [r2rcon-rs] [error] 
[2023-10-21] [08:35:45] [r2rcon-rs] [error] plugin panicked at src\lib.rs:50:18
[2023-10-21] [08:35:45] [r2rcon-rs] [error] full message:
[2023-10-21] [08:35:45] [r2rcon-rs] [error] panicked at 'a rcon cmd wasn't present', src\lib.rs:50:18
[2023-10-21] [08:35:45] [r2rcon-rs] [error] 

I updated SouthRPC to Plugins v3 but calling Cbuf_Execute causes a crash.
Had no luck debugging it thus far.

EDIT: Is a bug in wine caused by a debug trap. Hiding in x64dbg or not calling Execute fixes it. Uploaded a new version with it removed, tested on x64 Windows and Fedora 38.

SouthRPC V3 Source Code
SouthRPC V3 Plugin Zip

You can test SouthRPC is working by sending a request to
http://localhost:26503
with the body

{
	"jsonrpc": "2.0",
	"method": "execute_squirrel",
	"params": {
		"code": "printt(\"Hello World!\")"
	}
}

Here is a short form curl command:
curl --request POST --url http://localhost:26503/ --header 'Content-Type: application/json' --data '{"jsonrpc": "2.0", "method": "execute_squirrel", "params": {"code": "printt(\"Hello World!\")"}}'

NorthstarDLL/plugins/plugin_abi.h Outdated Show resolved Hide resolved
NorthstarDLL/plugins/plugin_abi.h Outdated Show resolved Hide resolved
@catornot
Copy link
Member Author

I tried testing with the given plugin but it just panics and gets the game stuck after the company logos

panicking is intended behavior since it didn't find the correct command line args and it's in a thread so it's fine.
The game probably got stuck because that plugin is outdated and shouldn't work after b292371.

Copy link
Contributor

@Jan200101 Jan200101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested already with SouthRPC.
Code is good.

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed working in testing together with R2Northstar/NorthstarMods#652 and R2Northstar/NorthstarDiscordRPC#10

Joined a bunch of servers and watched Discord activity status update

(No code review done in this review)

@GeckoEidechse GeckoEidechse removed the needs testing Changes from the PR still need to be tested label Nov 2, 2023
@GeckoEidechse GeckoEidechse added the almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge label Nov 2, 2023
Copy link
Contributor

@Jan200101 Jan200101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked over the code again and it still looks good to me.

@Jan200101 Jan200101 added READY TO MERGE This mergeable right now and removed needs code review Changes from PR still need to be reviewed in code almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Nov 5, 2023
@GeckoEidechse GeckoEidechse merged commit bb822b7 into R2Northstar:main Nov 6, 2023
2 checks passed
@GeckoEidechse
Copy link
Member

merged based on reviews ig

GeckoEidechse pushed a commit to R2Northstar/NorthstarMods that referenced this pull request Nov 6, 2023
Script component of plugins v3. See launcher PR for more info.
R2Northstar/NorthstarLauncher#472
GeckoEidechse pushed a commit to R2Northstar/NorthstarDiscordRPC that referenced this pull request Nov 6, 2023
Updates DiscordRPC plugin for v3. See launcher PR for more info:
R2Northstar/NorthstarLauncher#472
@catornot catornot deleted the pluginsv3 branch January 21, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY TO MERGE This mergeable right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants