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

Merge Jiarong's branch #6

Merged
merged 24 commits into from
Jun 9, 2023

Conversation

Splendide-Imaginarius
Copy link
Contributor

Fixes #5 . Also fixes a bug that broke the knee bend feature.

I've tested the branch for a few days, and it seems to work OK. Here are the caveats:

  • While I don't see any obvious bugs in the behavior, I don't have the necessary knowledge to properly review all of the code changes themselves.
  • I didn't test Studio mode at all, so if there are bugs there, I wouldn't have noticed.
  • There's a lot of code churn here, which might make KK and KKS codebases need to be merged #3 more difficult to solve.

Personally I don't think any of these caveats should block a merge, but it's not my repo, so my opinion matters little.

@Splendide-Imaginarius
Copy link
Contributor Author

There's a lot of code churn here, which might make #3 more difficult to solve.

There is a chance that I might try to unify the KK and KKS codebases sometime after this PR is merged, but this may or may not happen soon, and my C# skills may or may not be enough to do it successfully.

Copy link
Contributor

@ManlyMarco ManlyMarco left a comment

Choose a reason for hiding this comment

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

This looks like an almost completely different code base than what's currently in the repo. While it improves in a lot of areas it also regresses in others (e.g. no longer adding user-friendly maker controls, vastly increased code complexity, not using some of the newer features, etc.), so I'm hesitant to just blindly merge it.

To merge this with the KKS version it seems like the best way would be to scrap the current KKS codebase and port this version, adding any KKS specific hooks/tweaks that were added to the current version. You can find out what was changed in the KKS version in #2.

Stiletto/Stiletto.cs Outdated Show resolved Hide resolved
@ManlyMarco
Copy link
Contributor

Since this repository doesn't have active work happening on it and doesn't host releases so far, I guess there's no real harm in merging this and improving it from there. If you could handle the KKS port it would be great.

I added a legacy v1 branch in case the old version needs to be updated.

@ManlyMarco ManlyMarco merged commit fddefe0 into IllusionMods:master Jun 9, 2023
@Splendide-Imaginarius
Copy link
Contributor Author

While it improves in a lot of areas it also regresses in others (e.g. no longer adding user-friendly maker controls, vastly increased code complexity, not using some of the newer features, etc.)

Could you clarify which maker controls and newer features you noticed are missing? I'd be happy to try my hand at re-adding them, if it's within my skill level.

@ManlyMarco
Copy link
Contributor

Could you clarify which maker controls and newer features you noticed are missing? I'd be happy to try my hand at re-adding them, if it's within my skill level.

My bad, looks like the maker controls did get kept, just moved to a different file. In that case it's just about not using latest BepInEx and other nuget packages, which results in unnecessary reflection and other minor inefficiencies. Overall it's not a big issue, all it really does is make the codebase harder to understand.

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.

KK: "Save Preset" button missing following refactor
3 participants