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.

Sign up for GitHub

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

Jump to bottom

Plugin system v2 #343

Merged
merged 46 commits into from
Apr 11, 2023
Merged

Plugin system v2 #343

merged 46 commits into from
Apr 11, 2023

Conversation

emma-miler
Copy link
Member

Still a little bit work in progress. Feel free to review and comment. Will depend on a few more PR's across multiple repos

@pg9182 pg9182 self-requested a review November 25, 2022 02:14
@emma-miler emma-miler mentioned this pull request Nov 27, 2022
Merged
@emma-miler
Copy link
Member Author

Needs R2Northstar/NorthstarMods#532

@emma-miler
Copy link
Member Author

Use R2Northstar/NorthstarDiscordRPC#3 to test

@emma-miler
Copy link
Member Author

emma-miler commented Nov 30, 2022 •
edited
Loading

Some TODO's:

  • Fix abi.h comments
  • Fix load error strings
  • Fix logger not sinking to file
  • Fix filters
  • Fix function registration
  • Fix SQVM_CREATED (apparently)

@catornot
Copy link
Member

catornot commented Dec 3, 2022

I just want to mention that logs from PLUGINSYS don't get saved

console:
image

logs:
nslog2022-12-02 20-08-26.txt

@catornot
Copy link
Member

catornot commented Dec 4, 2022

logs from plugins aren't getting logged too
image
L logs:
nslog2022-12-03 20-57-48.txt

@emma-miler
Copy link
Member Author

@catornot putting this PR on the backburner for now while we switch toolchains. Will fix this issue after that is all done

@catornot
Copy link
Member

@emma-miler can you merge this pr -> #6 ( for rust support )

pg9182
pg9182 reviewed Mar 4, 2023
View reviewed changes
Copy link
Member

@pg9182 pg9182 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code seems good overall; haven't tested it, but if someone else does a proper review, I don't have any objection to merging it.

And this is definitely an overall improvement. I like the plugin request mechanism.

@GeckoEidechse GeckoEidechse mentioned this pull request Apr 10, 2023
Merged
GeckoEidechse
GeckoEidechse approved these changes Apr 10, 2023
View reviewed changes
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.

Tested for breakage and backwards compat on both client and dedi server and seems to work fine.

catornot
catornot approved these changes Apr 10, 2023
View reviewed changes
Copy link
Member

@catornot catornot left a comment

Choose a reason for hiding this comment

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

I have tested my plugins with the latest build and mods and they still work.

BobTheBob9
BobTheBob9 approved these changes Apr 10, 2023
View reviewed changes
Copy link
Member

@BobTheBob9 BobTheBob9 left a comment

Choose a reason for hiding this comment

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

looks fine at a glance, only issue is gamestate struct members using snake case (inconsistent with rest of codebase inside and out of pr), but overall fine

@GeckoEidechse
Copy link
Member

[...] only issue is gamestate struct members using snake case (inconsistent with rest of codebase inside and out of pr), but overall fine

Addressed in 432aaa6

@GeckoEidechse GeckoEidechse merged commit 450d0b1 into R2Northstar:main Apr 11, 2023
@GeckoEidechse GeckoEidechse removed needs testing Changes from the PR still need to be tested 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 Apr 11, 2023
pg9182 added a commit to pg9182/NorthstarLauncher that referenced this pull request Apr 19, 2023
@pg9182
Remove leftover LoadPlugins declaration
dcdc409 
Doesn't exist since 450d0b1 (R2Northstar#343).
@pg9182 pg9182 mentioned this pull request Apr 19, 2023
Merged
pg9182 added a commit that referenced this pull request Apr 19, 2023
@pg9182
Remove leftover LoadPlugins declaration (#449)
10b6839 
Doesn't exist since 450d0b1 (#343).
@GeckoEidechse GeckoEidechse mentioned this pull request Apr 23, 2023
Merged
uniboi added a commit to uniboi/NorthstarLauncher that referenced this pull request May 3, 2023
@uniboi
Revert "Plugin system v2 (R2Northstar#343)"
bc9808d 
This reverts commit 450d0b1.
GeckoEidechse added a commit that referenced this pull request May 4, 2023
@GeckoEidechse
Revert "Plugin system v2 (#343)"
7aec756 
This reverts commit 450d0b1.
pg9182 added a commit that referenced this pull request May 5, 2023
@pg9182
Revert "Plugin system v2 (#343)"
97df63b 
This reverts commit 450d0b1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jan200101 Jan200101 Jan200101 left review comments

@pg9182 pg9182 pg9182 left review comments

@BobTheBob9 BobTheBob9 BobTheBob9 approved these changes

@GeckoEidechse GeckoEidechse GeckoEidechse approved these changes

@catornot catornot catornot approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Northstar Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@emma-miler @catornot @GeckoEidechse @Jan200101 @BobTheBob9 @pg9182