Skip to content

Fixes issue #255 Companion affinity event on appearance change does not fire. #259

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

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

Bobbyclue
Copy link
Contributor

@Bobbyclue Bobbyclue commented Sep 23, 2023

Swapped the menu registration from "LooksMenu" to "ChargenMenu". I also made an optional parameter to override the typical inability of companions to comment while the player is in dialogue since all appearance changes occur in dialogue. This will only affect the appearance change dialogue which previously did not play at all.

I considered listening for the dialogue menu to close to play the relevant lines, however there is no way to directly check if the dialogue menu is open through papyrus, only if the player is in dialogue which is technically different and could result in the line misfiring. This is barring polling it constantly, but that did not seem worthwhile.

This fix is not retroactive, a new save will be needed to have the registration function fire again.

Decompiled with Champollion 1.3.1.

Attached the compiled script below for testing:
companionaffinityeventsscript.zip

@Noggog
Copy link
Member

Noggog commented Sep 23, 2023

Hey @Bobbyclue! Thanks for the PR!

This is the first papyrus related PR, and on seeing it, I'm wondering if it would be good to iron out some procedures for adding new script tweaks.

By that, I mean it's hard to pick out what changes you've applied to the script, because the whole file is marked as new. I think it would be a lot clearer if new scripts are first committed without any changes, as a baseline, and then commits can follow with tweaks to the vanilla. This way, it will be clear how the file looked initially, and then the changes on top will be more easily digestible.

Would this be something you can try doing to see if it works well? Either close this PR and open another one following that pattern, and/or just make the adjustments and then force push the branch?

@Bobbyclue
Copy link
Contributor Author

Sure thing, I'll get something together in a little.

@Bobbyclue
Copy link
Contributor Author

@Noggog the new base has been pushed and the new changes on top of that.

@FlayaN
Copy link
Member

FlayaN commented Sep 23, 2023

Don't we also want to get rid of the 091ca19a006730fc7a52ddd35b2a829943eb1128 commit? So it's only 2 commits?

@Noggog
Copy link
Member

Noggog commented Sep 23, 2023

The baseline commit is still your original modified version. You've added additional commits reverting it to the vanilla script and then readding your changes, but that's not exactly what we're meaning, as the baseline still is the modified version.
image

In order to get the effect we're hoping, the easiest way is to do the PR "from scratch". Make a new branch. Commit the vanilla version, add your changes. That way, vanilla version will truly be the baseline. Can either make a new PR from that branch, or force push if you've reused the same branch driving this PR.

If all that's too over your head, don't worry about it too much

@Noggog
Copy link
Member

Noggog commented Sep 23, 2023

Main thing we're waiting on for this PR is to just get some of the @Starfield-Community-Patch/qa systems revved up:

  • Get someone to reproduce the issue
  • Have them use this fix and confirm it does the fix

Unfortunately, I'm a bit too involved in other projects to do this myself. Hoping to get the community as a whole more involved in phases like this

@Noggog Noggog added qa-needs-reproducing Needs someone from the QA team to reproduce the original bug qa-fix-needs-testing Needs someone from the QA team to test the fix labels Sep 23, 2023
@Noggog
Copy link
Member

Noggog commented Sep 23, 2023

Sit tight @Bobbyclue. Might be a little bit before we merge this in, just because we're still figuring out logistics of what goes into actually approving a fix to be merged in. Feel free to chat a bit more on the discord if you've got thoughts or questions!

@Pickysaurus
Copy link
Member

I'll have a look at #255 to better understand what you're trying to fix. I'm not super confident we want to include it with all the guff Champollion adds to the file and as Noggog said it's hard to compare it to the original to see exactly what you changed. Could you maybe provide a little rundown of which lines you changed?

@FlayaN
Copy link
Member

FlayaN commented Sep 24, 2023

Could you maybe provide a little rundown of which lines you changed?

You can check the changes by looking at this specific commit as a temporary solution. Still the correct delta
fe7fbce

The only issue right now is that the initial commit is still present

@Bobbyclue
Copy link
Contributor Author

Yes apologies there, not super familiar with github. What would be the best way to remove the initial commit? I can also close this and open a new pr, but didn't want to lose the discussion on this one.

In the meantime @Pickysaurus the lines I changed from the original decompiled script from champollion 1.3.1 are as follows:
196 - Swapped the registration to "LooksMenu" to "ChargenMenu"
313 - Again swapped the check to ChargenMenu
314 - Added additional parameter to SendAfinityEvent function call which I'll explain below
423 - Added additional parameter to SendAfinityEvent function that will override the player dialogue exclusion if true, defaults to false
424 - Made PlayerInDialogue var default to false rather than a bool based on player dialogue state
427 - If ignorePlayerInDialogue is false, set PlayerInDialogue to the player dialogue state. Otherwise leave it as false.

In addition, I amended the decompiled source on lines 152 and 293 to allow recompilation with Caprica. Champollion decompiles setting the script into a none state as GotoState("None"), however Caprica (and the CK in past games iirc) require this to be written as GotoState("").

@Noggog
Copy link
Member

Noggog commented Sep 25, 2023

Yes apologies there, not super familiar with github. What would be the best way to remove the initial commit?

This video is me doing it in GitKraken, but what you want to do is "drop" the first commit.

gitkraken_IO7VCB78H6.mp4

@Bobbyclue
Copy link
Contributor Author

Thanks for the detailed rundown! I think it should be all good to go now.

@Pickysaurus
Copy link
Member

@Bobbyclue Thanks for being patient with this. I'm still a bit iffy about the Champollion guff in the file, but it's clear the fix works. When the CK releases we might want to re-do this fix with the proper source PSC?

@Pickysaurus Pickysaurus merged commit aaed1cf into Starfield-Community-Patch:main Sep 26, 2023
@Bobbyclue
Copy link
Contributor Author

When the CK releases we might want to re-do this fix with the proper source PSC?

I absolutely agree, the fix should be reimplemented when we get access to the real source files. For now though Champollion does the job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qa-fix-needs-testing Needs someone from the QA team to test the fix qa-needs-reproducing Needs someone from the QA team to reproduce the original bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants