IllusionMods/Stiletto
1
0
Fork 0
Code Issues Pull requests Projects Releases 1 Packages Wiki Activity Actions

KKS port and project code merge #9

Merged
IDontHaveIdea merged 14 commits from kks-port-merge into master 2023-08-13 14:50:46 +00:00
Conversation 12 Commits 14 Files changed 38 +381 -87
IDontHaveIdea commented 2023-07-29 04:40:05 +00:00 (Migrated from github.com)
Copy link

Hi,

Just:

  • added Stiletto.Core project
  • moved code from Stiletto project to Stiletto.Core updated dependencies
  • added configuration for conditional compilation
  • added Stiletto.KKS updated dependencies
  • added conditional compilation steps to compile the plug-in for KKS

Did not look in depth into Stiletto_KKS since the code is so different it was easier to work on the Stiletto code base.

I don't use Studio so no testing there. It seems to work fine in KKS.

For the method OnHeelInfoUpdate had to use try {} block. It gives and error the second time you invoke Maker in the same
game session. Don't have KK installed for testing this change in the code for KK. Though logic dictates that the problem should be present in KK also. The nullchecks fix if active will permit the plug-in to continue to run as is the case in KKS.

Hi, Just: - added Stiletto.Core project - moved code from Stiletto project to Stiletto.Core updated dependencies - added configuration for conditional compilation - added Stiletto.KKS updated dependencies - added conditional compilation steps to compile the plug-in for KKS Did not look in depth into Stiletto_KKS since the code is so different it was easier to work on the Stiletto code base. I don't use Studio so no testing there. It seems to work fine in KKS. For the method OnHeelInfoUpdate had to use try {} block. It gives and error the second time you invoke Maker in the same game session. Don't have KK installed for testing this change in the code for KK. Though logic dictates that the problem should be present in KK also. The nullchecks fix if active will permit the plug-in to continue to run as is the case in KKS.
ManlyMarco (Migrated from github.com) requested changes 2023-08-02 21:23:02 +00:00
ManlyMarco (Migrated from github.com) left a comment
Copy link

Thank you for the PR! The changes look good overall, just some minor issues to button up.

If you are going with the KK codebase, are you sure the KKS version didn't have anything worth keeping? e.g. fixes or UI changes.

Thank you for the PR! The changes look good overall, just some minor issues to button up. If you are going with the KK codebase, are you sure the KKS version didn't have anything worth keeping? e.g. fixes or UI changes.
Stiletto.Core/StilettoMakerGUI.cs
@ -0,0 +171,4 @@
MakerHeelInfoProcess(heel => StilettoContext.CustomHeelProvider.Save(heel.heelName, new CustomHeel(heel))
));
button_Reload.OnClick.AddListener(StilettoContext.ReloadConfigurations);
ManlyMarco (Migrated from github.com) commented 2023-08-02 21:16:08 +00:00
Copy link

Wouldn't using InsideAndLoaded fix this issue? A blank catch is not great because it will hide unexpected issues, at least log the exceptions once caught.

You can also use MakerAPI.MakerExiting to clean up any hooks or fields that were set when entering maker so that you return to the clean initial state.

Wouldn't using InsideAndLoaded fix this issue? A blank catch is not great because it will hide unexpected issues, at least log the exceptions once caught. You can also use `MakerAPI.MakerExiting` to clean up any hooks or fields that were set when entering maker so that you return to the clean initial state.
Stiletto.KKS/Properties/AssemblyInfo.cs
@ -0,0 +1,10 @@
using System.Runtime.InteropServices;
ManlyMarco (Migrated from github.com) commented 2023-08-02 21:18:13 +00:00
Copy link

Assembly version is not set. It's better to move AssemblyInfo into the shared project and have both game versions share the same file.

Assembly version is not set. It's better to move AssemblyInfo into the shared project and have both game versions share the same file.
Stiletto.KKS/Stiletto.KKS.csproj
@ -0,0 +1,165 @@
<?xml version="1.0" encoding="utf-8"?>
ManlyMarco (Migrated from github.com) commented 2023-08-02 21:19:10 +00:00
Copy link
    <DebugType>pdbonly</DebugType>

There are some inconsistent settings between KK and KKS projects, nothing major.

```suggestion <DebugType>pdbonly</DebugType> ``` There are some inconsistent settings between KK and KKS projects, nothing major.
IDontHaveIdea (Migrated from github.com) reviewed 2023-08-03 22:28:58 +00:00
Stiletto.KKS/Stiletto.KKS.csproj
@ -0,0 +1,165 @@
<?xml version="1.0" encoding="utf-8"?>
IDontHaveIdea (Migrated from github.com) commented 2023-08-03 22:28:58 +00:00
Copy link

I work this and look into the KKS that is quite different. There was some recent work done one the KK branch that why I pick that one.

I work this and look into the KKS that is quite different. There was some recent work done one the KK branch that why I pick that one.
IDontHaveIdea (Migrated from github.com) reviewed 2023-08-04 01:52:56 +00:00
Stiletto.Core/StilettoMakerGUI.cs
@ -0,0 +171,4 @@
MakerHeelInfoProcess(heel => StilettoContext.CustomHeelProvider.Save(heel.heelName, new CustomHeel(heel))
));
button_Reload.OnClick.AddListener(StilettoContext.ReloadConfigurations);
IDontHaveIdea (Migrated from github.com) commented 2023-08-04 01:52:56 +00:00
Copy link

Yes this does fix the issue.

Yes this does fix the issue.
IDontHaveIdea (Migrated from github.com) reviewed 2023-08-04 01:55:45 +00:00
Stiletto.KKS/Properties/AssemblyInfo.cs
@ -0,0 +1,10 @@
using System.Runtime.InteropServices;
IDontHaveIdea (Migrated from github.com) commented 2023-08-04 01:55:44 +00:00
Copy link

Addressed this the way I usually do on my projects
Properties/StilettoInfo.cs
I have a name for debug Stiletto (Debug) to make it easier to now if version is released by mistake.

Addressed this the way I usually do on my projects Properties/StilettoInfo.cs I have a name for debug Stiletto (Debug) to make it easier to now if version is released by mistake.
IDontHaveIdea (Migrated from github.com) reviewed 2023-08-04 02:07:45 +00:00
Stiletto.KKS/Stiletto.KKS.csproj
@ -0,0 +1,165 @@
<?xml version="1.0" encoding="utf-8"?>
IDontHaveIdea (Migrated from github.com) commented 2023-08-04 02:07:45 +00:00
Copy link

Also did a fast check on the Stiletto_KKS project the code is to different. Right now I won't be able to do a good analysis to see what code base is more efficient. I won't promise anything but I'll try to make time.

Also did a fast check on the Stiletto_KKS project the code is to different. Right now I won't be able to do a good analysis to see what code base is more efficient. I won't promise anything but I'll try to make time.
IDontHaveIdea commented 2023-08-04 05:25:46 +00:00 (Migrated from github.com)
Copy link

Just a fast check the code for the Stiletto_KKS compiles as is for KK but I can't test if it works in KK.

Just a fast check the code for the Stiletto_KKS compiles as is for KK but I can't test if it works in KK.
ManlyMarco commented 2023-08-04 08:50:52 +00:00 (Migrated from github.com)
Copy link

Alright, let me know when it's ready for a review.

Alright, let me know when it's ready for a review.
Splendide-Imaginarius commented 2023-08-11 08:17:32 +00:00 (Migrated from github.com)
Copy link

Nice work! I'm happy to give it a try on KK and see if anything is obviously broken.

Nice work! I'm happy to give it a try on KK and see if anything is obviously broken.
Splendide-Imaginarius commented 2023-08-12 12:52:45 +00:00 (Migrated from github.com)
Copy link

No obvious breakage in about an hour of testing in both KK and KKS (mostly KK). I'll try to do some additional testing in the next few days, but so far LGTM.

No obvious breakage in about an hour of testing in both KK and KKS (mostly KK). I'll try to do some additional testing in the next few days, but so far LGTM.
Splendide-Imaginarius commented 2023-08-13 05:55:46 +00:00 (Migrated from github.com)
Copy link

Tested in KK for another couple hours, no issues connected to this PR. I did find a bug in the shoe warp feature, but I'm 90% sure that's a bug in master branch that I accidentally introduced while optimizing that feature at the last minute, not this PR's fault -- I'll send in a PR to fix that after this PR is merged, so that we don't have to deal with merge conflicts.

@IDontHaveIdea is there anything you still need to do before this gets merged?

Tested in KK for another couple hours, no issues connected to this PR. I did find a bug in the shoe warp feature, but I'm 90% sure that's a bug in master branch that I accidentally introduced while optimizing that feature at the last minute, not this PR's fault -- I'll send in a PR to fix that after this PR is merged, so that we don't have to deal with merge conflicts. @IDontHaveIdea is there anything you still need to do before this gets merged?
ManlyMarco (Migrated from github.com) approved these changes 2023-08-13 14:50:10 +00:00
ManlyMarco commented 2023-08-13 14:51:40 +00:00 (Migrated from github.com)
Copy link

@Splendide-Imaginarius I merged it since the issues appear to have been resolved. Feel free to send PRs now :)

@Splendide-Imaginarius I merged it since the issues appear to have been resolved. Feel free to send PRs now :)
Sign in to join this conversation.
Reviewers
No reviewers
ManlyMarco
Labels
Clear labels   
bug

Something isn't working

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
question

Further information is requested

  
wontfix

This will not be worked on

No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
IllusionMods/Stiletto!9
No description provided.