[MainGame][KK/KKS] Pov changes#112
[MainGame][KK/KKS] Pov changes#112obedientGolem wants to merge 4 commits intoIllusionMods:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors PoV (impersonation) configuration to be simpler/more consistent, and revises PoV head/face hiding behavior—especially around “away/remote attach” scenarios—to make it more predictable.
Changes:
- Renamed/normalized PoV config entries and reworked hide-head settings into a
[Flags]enum. - Replaced
FlightSpeedusage withPovFlightSpeedand replaced movement-type config with aPovFlightboolean toggle. - Updated PoV head/face hide logic to better control proactive enabling/hiding and support new modifiers (e.g.,
Always,Away).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| SharedGame/Handlers/MouthGuide.cs | Uses the new PoV transition speed setting for disengage rotation smoothing. |
| Shared/Settings/KoikSettings.cs | Renames PoV config keys/descriptions and introduces the new flags-based hide-head setting plus PovFlight/PovFlightSpeed. |
| Shared/Camera/SmoothMover.cs | Switches movement smoothing to use PovFlightSpeed. |
| Shared/Camera/Pov.cs | Implements new hide-head flags semantics and updates transition logic to use PovFlight + PovFlightSpeed. |
| Shared/Camera/MoveToPoi.cs | Switches lerp multiplier to use PovFlightSpeed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Shared/Camera/Pov.cs
Outdated
|
|
||
| // Setting doesn't force to hide when Away – show full head because look better. | ||
| else | ||
| SetHeadActive(_target, true); |
There was a problem hiding this comment.
SetHeadActive(_target, true); is already always called a few lines above, is this intended?
There was a problem hiding this comment.
This is a trade off between code readability and functionality. I'd rather opt to rarely call a method twice. The method is very far from the update cycle, shouldn't affect anything much.
Your call.
There was a problem hiding this comment.
In that case either add a comment describing this or remove the redundant call. If someone looks at this in the future they would assume this is most likely an error and could indicate a bug.
Shared/Settings/KoikSettings.cs
Outdated
| None = 0, | ||
| Face = 1 << 0, | ||
| Head = 1 << 1, | ||
| Always = 1 << 2, | ||
| Away = 1 << 3, |
There was a problem hiding this comment.
This is just as confusing as it was before. I would expect Always to be a combination of all other flags, not a separate flag (Face | Head | Away), same with Head (Face | Hair?).
What Away does is not obvious at all (or how it's affected by Always), even with the description on the setting. It seems like it should be a separate setting since it appears to be a different function than choosing what parts are hidden.
You can add DescriptionAttribute to the enum members to show a more descriptive name in config manager.
There was a problem hiding this comment.
It seems like it should be a separate setting since it appears to be a different function than choosing what parts are hidden.
I would argue that those options are for the same feature and thus should be combined (and there are already too many settings, so multiplying them is not optimal).
How they work:
User chooses what to hide and how to hide
Face- hides only face, leaves hair visibleHead- pretty much overrides face and hides the whole headAlways- stops checking if the camera is close on each frame and hides constantly, but still shows during the grip move.OnGripMove(renamedAway) - hides the face/head during the grip move. Not sure who would need this option but this behavior could be achieved before, so I decided to keep it.
To reiterate, the plugin has many many settings, descriptions are lengthy and all this is accessed in the VR, where doing all the precision stuff is very hard and cumbersome. Then we have people who encounters this and may opt out of vr completely or out of the settings and then complain that something isn't there.
I'd say this is the fault of the plugin, of settings in particularity, and so I'm trying to address it bit by bit.
I literally every once in a while explain to people settings of the plugin via dm, that's telling.
There was a problem hiding this comment.
An alternative of making it into 3 separate settings horrifies me, too much clutter to navigate and understand correlation between settings.
There was a problem hiding this comment.
This is just as confusing as it was before. I would expect Always to be a combination of all other flags, not a separate flag (Face | Head | Away), same with Head (Face | Hair?).
Hope it makes sense with renamed flag, because for me it clicks perfectly now.
There was a problem hiding this comment.
It seems like 2 settings rather than 3. One to select what to hide (nothing/disabled, only face, whole head) and when to hide (close to camera, away from camera when not grip moving, always). This way it would be much simpler to understand since it's just 2 simple dropdown settings.
There was a problem hiding this comment.
Does this approach look better?
Also I must remind that perfectionism in nearly all fields of human endeavor is viewed as an ailment, not a virtue. Project that do strive towards it either have unlimited resources to rely on or never finish.
There's even the saying "Good enough is perfect".
There was a problem hiding this comment.
If the text actually fits in the UI, this looks good. It's much clearer what the settings do.
Off topic: Perfectionism should not be seen as a negative, doing so invites the opposite which is just as bad. Perfectionism is just a tool that needs practice to be used correctly.
Since the goal of this PR is improving user experience it makes sense to give more attention to that part in particular, otherwise you can end up trading a bad for a different bad.
|
Changed the name of a flag from |
|
Damn after all the changes it doesn't work the intended way. |
Description
Motivation and Context
Got horrified by the pov settings while trying to fiddle with them so that facilitated the update.
And the way pov hides head wasn't exactly straightforward or easy to understand, while also lacking certain options, so this PR fixes that.
How Has This Been Tested?
This one tested in full, works as intended.
Types of changes